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.

Sign up for GitHub

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

Jump to bottom

field checks done too early causing error #1147

Closed
jamesmurphy-mc opened this issue Jun 13, 2023 · 3 comments · Fixed by #1401
Closed

field checks done too early causing error #1147

jamesmurphy-mc opened this issue Jun 13, 2023 · 3 comments · Fixed by #1401

Comments

@jamesmurphy-mc
Copy link
Contributor

Attrs does some checks on fields like no defaulted fields coming before non-defaulted fields before running custom field transformers. This is a problem when a field transformer intends to modify the fields but gets an error before being able to apply their modification.
For example, this field transformer reorders fields so that in fact the defaulted field x comes after the non-defaulted field y. But attrs errors before the transformer is ever called.

def order_by_metadata(cls, fields):
    fields.sort(key=lambda x: x.metadata["field_order"])
    return fields

@define(field_transformer=order_by_metadata)
class A:
    x: int = field(metadata={"field_order": 1}, default=0)
    y: int = field(metadata={"field_order": 0})
Traceback (most recent call last):
  File "...", line 426, in <module>
    class A:
  File ".../attr/_next_gen.py", line 146, in wrap
    return do_it(cls, True)
  File ".../attr/_next_gen.py", line 90, in do_it
    return attrs(
  File ".../attr/_make.py", line 1603, in attrs
    return wrap(maybe_cls)
  File ".../attr/_make.py", line 1499, in wrap
    builder = _ClassBuilder(
  File ".../attr/_make.py", line 657, in __init__
    attrs, base_attrs, base_map = _transform_attrs(
  File ".../attr/_make.py", line 566, in _transform_attrs
    raise ValueError(
ValueError: No mandatory attributes allowed after an attribute with a default value or factory.  Attribute in question: Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({'field_order': 0}), type='int', converter=None, kw_only=False, inherited=False, on_setattr=None, alias=None)

I think these checks should be done after custom field transformers.
@mara004
Copy link

mara004 commented Jul 19, 2024 •
edited
Loading

I concur, see also #1169 (comment)

@hynek
Copy link
Member

hynek commented Jul 24, 2024

@sscherfke transformers are your labor of love – is this realistic without adding a whole abstraction layer over this?

@mara004
Copy link

mara004 commented Aug 19, 2024 •
edited
Loading

is this realistic without adding a whole abstraction layer over this?

Not sure what you mean -- why isn't it sufficient to just move the order check after the field transformer has been applied?

IIUC, field transformers can already do arbitrary rearranging, as long as the initial field order is valid.
But all that matters should be the final order, not the initial one, right? Conversely, a field transformer that results in an invalid order would currently slip past the check.

That said, why do we need the manual check in the first place? Python already gives us a pretty good exception when attempting to evaluate the incorrect code. Picking up the above example:

def order_by_metadata(cls, fields):
    fields.sort(key=lambda x: x.metadata["field_order"])
    return fields

@define(field_transformer=order_by_metadata)
class A:
    x: int = field(metadata={"field_order": 1})
    y: int = field(metadata={"field_order": 0}, default=0)

This yields the following error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/me/.local/lib/python3.11/site-packages/attr/_next_gen.py", line 144, in wrap
    return do_it(cls, True)
           ^^^^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_next_gen.py", line 90, in do_it
    return attrs(
           ^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1715, in attrs
    return wrap(maybe_cls)
           ^^^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1694, in wrap
    builder.add_init()
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1090, in add_init
    _make_init(
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 2181, in _make_init
    init = _make_method(
           ^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 345, in _make_method
    _compile_and_eval(script, globs, locs, filename)
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 317, in _compile_and_eval
    bytecode = compile(script, filename, "exec")
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<attrs generated init __main__.A>", line 1
    def __init__(self, y=attr_dict['y'].default, x):
                                                 ^
SyntaxError: non-default argument follows default argument

jamesmurphy-mc added a commit to jamesmurphy-mc/attrs that referenced this issue Jan 31, 2025
@jamesmurphy-mc
Perform attr order checks after field transformer.
585e045 
Fixes: python-attrs#1147
@jamesmurphy-mc jamesmurphy-mc mentioned this issue Jan 31, 2025
Merged
14 tasks
github-merge-queue bot pushed a commit that referenced this issue Feb 9, 2025
@jamesmurphy-mc @pre-commit-ci @hynek
Perform attr order checks after field transformer. (#1401)
e048efc 
* Perform attr order checks after field transformer.

Fixes: #1147

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix assert style in test_hooks

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Hynek Schlawack <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Perform attr order checks after field transformer. jamesmurphy-mc/attrs
3 participants
@hynek @mara004 @jamesmurphy-mc