Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add literal string support to include and exclude filters #1068

Merged
merged 13 commits into from Apr 14, 2023

Conversation

lqhuang
Copy link
Contributor

@lqhuang lqhuang commented Dec 4, 2022

Summary

It's a little too cumbersome that helper util filters only accepts type and Attribute as input, if there are too many fields to include or exclude using asdict.

For example,

from attrs import asdict, fields

@define
Class D:
    a = attr.ib()
    b = attr.ib()
    c = attr.ib()
    d = attr.ib()
    e = attr.ib()
    f = attr.ib()

data = D(...)
fs = fields(D)
asdict(data, filter=exclude(fs.a, fs.c, fs.f))

This PR adds literal string name of Attribute as input argument to exclude and include which provides a more convenience approach for usages of filter. The following codes are similar to the above:

from attrs import asdict

@define
Class D:
    a = attr.ib()
    b = attr.ib()
    c = attr.ib()
    d = attr.ib()
    e = attr.ib()
    f = attr.ib()

data = D(...)
asdict(data, filter=exclude("a", "c", "f"))

And typo issues of input arguments wouldn't cause extra side effects.

This's my first PR to attrs, please feel free to inform me where I'm wrong and what I missed. Thanks!

Pull Request Check List

  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@lqhuang lqhuang marked this pull request as ready for review December 4, 2022 13:10
@Tinche
Copy link
Member

Tinche commented Dec 5, 2022

It's not really equivalent though. See this example with a typo:

from attrs import asdict, define, fields
from attrs.filters import exclude


@define
class User:
    id: str
    password: str

data = User("id", "s3cret")
fs = fields(User)

asdict(data, filter=exclude("passwrd"))
asdict(data, filter=exclude(fs.passwrd))

When passing in a string, the typo goes unnoticed and the password field is serialized.

@lqhuang
Copy link
Contributor Author

lqhuang commented Dec 5, 2022

@Tinche Thanks for your reply 😄.

Yeah, you're right, fields will raise an AttributeError when the field doesn't exist. I should update "totally equivalent" to "similar". Current implementation is the simplest way with no overhead, while no extra side effect means no another unexpected behavior.

Could we document this potential consequence explicitly to warn users? IMHO, it probably depends on whether attrs should provide full soundness checks?

I also have an extreme radical thought that perhaps I could implement a generic type signature in pyi level to check correctness of field name string without runtime cost.

@lqhuang lqhuang force-pushed the accept-str-for-excludes-and-includes branch 3 times, most recently from fcb007e to f609858 Compare February 4, 2023 12:01
@lqhuang
Copy link
Contributor Author

lqhuang commented Feb 4, 2023

Rebase to the latest head ref 9cf2ed5 (2023-02-04)

@lqhuang lqhuang force-pushed the accept-str-for-excludes-and-includes branch from c30f382 to d22135b Compare April 12, 2023 03:53
@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 12, 2023

Rebase from head ref 683d056 (2023-04-12)

@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 12, 2023

Add a note to document incorrect typo behaviors while using literal string in filters

:::{note}
Though using string names directly is convenient, it's not safe in typo issues.
Using `fields()` to get attributes is worth being recommended in most cases.

```{doctest}
>>> asdict(
...     User("jane", "s33kred", 42),
...     filter=filters.exclude("passwd")
... )
{'login': 'jane', 'password': 's33kred', 'id': 42}

>>> asdict(
...     User("jane", "s33kred", 42),
...     filter=fields(User).passwd
... )
Traceback (most recent call last):
...
AttributeError: 'UserAttributes' object has no attribute 'passwd'. Did you mean: 'password'?
```
:::

@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 12, 2023

@hynek Could you let actions run again? Or you may have other suggestions?

Thanks :)

Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some comments.

docs/examples.md Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
src/attr/filters.py Outdated Show resolved Hide resolved
src/attr/filters.py Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@lqhuang lqhuang requested a review from chrysle April 12, 2023 12:54
@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 12, 2023

@chrysle Thank you for review. Updated.

changelog.d/1068.change.rst Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
src/attr/filters.py Outdated Show resolved Hide resolved
@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 14, 2023

@hynek Thanks for your advices. Updated

@lqhuang lqhuang requested a review from hynek April 14, 2023 07:40
docs/examples.md Outdated
@@ -240,8 +241,34 @@ For the common case where you want to [`include`](attrs.filters.include) or [`ex
>>> asdict(C("foo", "2", 3),
... filter=filters.include(int, fields(C).x))
{'x': 'foo', 'z': 3}

>>> asdict(C("foo", "2", 3),
... filter=filters.include(fields(C).x), "z")

This comment was marked as resolved.

docs/examples.md Outdated Show resolved Hide resolved
changelog.d/1068.change.md Outdated Show resolved Hide resolved
src/attr/filters.py Outdated Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
Co-authored-by: chrysle <fritzihab@posteo.de>
@hynek hynek enabled auto-merge (squash) April 14, 2023 08:46
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this should be fine now.

@hynek hynek merged commit c4c6fdd into python-attrs:main Apr 14, 2023
26 checks passed
@lqhuang lqhuang deleted the accept-str-for-excludes-and-includes branch April 17, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants