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

Remove PrimitiveField and align AsyncField with Field #11231

Merged
merged 4 commits into from
Nov 24, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Nov 23, 2020

Broken out of #10932.

The difference between AsyncField and PrimitiveField is fairly artificial and it creates more complexity than we need. Really, the only difference is a) storing the Address and b) an expression of intent.

This makes Field act like PrimitiveField, which is now deleted. AsyncField now uses the same naming as PrimitiveField used to, i.e.value instead of sanitized_raw_value and compute_value() instead of sanitize_raw_value().

This also cleans up some of the relevant target_test.py. We were testing something that's no longer worth it, as it's outside the scope of the code and we have multiple places we test this pattern. We add a new test that __eq__ and __hash__ are correct.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Remove an unnecessary test that set up an async field rule. It's redundandant to do that - async fields are very well used now, and there's really nothing special about them _except_ for adding an `Address` field + conventions. You could set up a rule for a non-async field too.

Also test that __eq__ and __hash__ still work properly.

[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor Author

I was really excited by an idea to make AsyncField a mixin so that it could be combined with the templates like StringField. I don't like that right now we have to copy the templates, e.g. AsyncStringSequenceField.

But I couldn't get the multiple inheritance, dataclasses, and @frozen_after_init working properly with __hash__, __eq__, and __repr__.

@Eric-Arellano
Copy link
Contributor Author

But I couldn't get the multiple inheritance, dataclasses, and @frozen_after_init working properly with hash, eq, and repr.

Honestly, we should probably just not use dataclasses for Field. I think they ironically add more cognitive overhead -- too much magic. We're abusing them pretty badly. I'll try changing that, but will save it for a followup.

@Eric-Arellano
Copy link
Contributor Author

I was really excited by an idea to make AsyncField a mixin so that it could be combined with the templates like StringField.

Huzzah! Removing dataclasses fixes it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 150809a on Eric-Arellano:async-field into 85f43eb on pantsbuild:master.

@Eric-Arellano Eric-Arellano merged commit 82fc6ca into pantsbuild:master Nov 24, 2020
@Eric-Arellano Eric-Arellano deleted the async-field branch November 24, 2020 01:33
Eric-Arellano added a commit that referenced this pull request Nov 24, 2020
By doing this, it's now possible to combine any arbitrary field template with being an async field. Previously, we duplicated the templates, like `AsyncStringSequenceField`. This also builds off of #11231 to make async fields far less magical - literally, all they do is add an `address: Address` field.

To land this, we remove both `@dataclass` and `ABC` from `Field`, which were causing unnecessary confusion and messing up inheritance. The dataclass was only generating a custom `__eq__` and `__hash__`, which is easy to set explicitly. We also remove both things from `Target` for simplicity.

[ci skip-rust]
[ci skip-build-wheels]
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.

3 participants