erratum: rename "from_" to "from" (with alias available) #108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Erratum objects have a "from" field, which unfortunately clashes with a Python keyword. It's not possible by default to use an attribute name which matches a Python keyword. As a workaround for this, the attribute was previously named simply "from_". Although that's a simple solution, while writing code using this library I've come to believe it was a mistake. The problem is that these push item classes are mostly used for serialization and deserialization of objects coming from various sources, all of which are going to be using the preferred attribute name, "from". This implies that, with the old solution, almost every usage of ErratumPushItem class would have to be accompanied with some "from" <=> "from_" renaming hack. In the worst case, this renaming could be forgotten and the "from_" name would leak into other tools and services due to this library. Hence, although it requires a bit of tricky code extending attrs to get it working, I believe it's worthwhile to complicate this library a bit so that the attribute can be genuinely named "from". This means the complexity due to the keyword clash can be kept within this library rather than spilling into every user of the ErratumPushItem class. While constructing or reading instances of the class, the "from_" name may still be used as an alias, so this is a backwards-compatible change.
|
Due to "weirdness" of this change, I want to keep it a draft until I have a chance to actually try it with Pub. |
|
Have experimented with this in Pub, seems to be working fine. |
chandwanitulsi
approved these changes
Nov 12, 2020
querti
added a commit
to querti/pushsource
that referenced
this pull request
Jan 5, 2023
Since the attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.
[1] python-attrs/attrs#950
[2] release-engineering#108
querti
added a commit
to querti/pushsource
that referenced
this pull request
Jan 5, 2023
Since new attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.
[1] python-attrs/attrs#950
[2] release-engineering#108
querti
added a commit
to querti/pushsource
that referenced
this pull request
Jan 5, 2023
Since new attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.
[1] python-attrs/attrs#950
[2] release-engineering#108
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Erratum objects have a "from" field, which unfortunately clashes
with a Python keyword.
It's not possible by default to use an attribute name which
matches a Python keyword. As a workaround for this, the attribute
was previously named simply "from_".
Although that's a simple solution, while writing code using this
library I've come to believe it was a mistake.
The problem is that these push item classes are mostly used for
serialization and deserialization of objects coming from various
sources, all of which are going to be using the preferred attribute
name, "from".
This implies that, with the old solution, almost every usage of
ErratumPushItem class would have to be accompanied with some
"from" <=> "from_" renaming hack. In the worst case, this renaming
could be forgotten and the "from_" name would leak into other tools
and services due to this library.
Hence, although it requires a bit of tricky code extending attrs
to get it working, I believe it's worthwhile to complicate this
library a bit so that the attribute can be genuinely named "from".
This means the complexity due to the keyword clash can be kept
within this library rather than spilling into every user of the
ErratumPushItem class.
While constructing or reading instances of the class, the "from_"
name may still be used as an alias, so this is a
backwards-compatible change.