-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
Simplify BUILD file parsing #10298
Simplify BUILD file parsing #10298
Conversation
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
[ci skip-rust-tests]
include_patterns = set( | ||
itertools.chain.from_iterable( | ||
address_spec.make_glob_patterns(address_mapper) for address_spec in address_specs | ||
) | ||
) | ||
snapshot = await Get( | ||
Snapshot, | ||
PathGlobs( | ||
globs=(*include_patterns, *(f"!{p}" for p in address_mapper.build_ignore_patterns)) | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was inlined from _address_spec_to_globs
, which was only being used here.
@@ -1,57 +1,47 @@ | |||
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of the changes:
- Removed the JSON parser examples. Those were testing functionality that we don't use in production, like strong references to other structs. Replaced with some integration-style tests for BUILD file parsing.
- Rewrote tests from unittest to Pytest style.
- Added new tests for finding
BuildFileAddress
and forAddressesWithOrigins -> Addresses
.
return HydratedStructs(tacs) | ||
|
||
|
||
class AddressMapperTest(unittest.TestCase, SchedulerTestBase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was testing things that are beyond the scope of mapper.py
. The functionality is roughly covered by the new tests in build_files_test.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
|
||
@dataclass(frozen=True) | ||
class HydratedTargetAdaptor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intermediate class necessary anymore, or could we request TargetAdaptor
directly now that that class is concrete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not. Although I like it to distinguish between a raw TargetAdaptor
vs. one that's been hydrated, e.g. the dependencies
field being normalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's a bit of misnomer now. "hydrated" used to mean "embedded addressables have been inlined/expanded" (the recursive stuff we used to do there). Now it's pure parsing, which isn't particularly heavy. Anywho.
We no longer need all the flexibility from the engine experiments. For the past few years, the only
Struct
we've ever had is aTargetAdaptor
, and it doesn't refer to other Structs beyond weak references in thedependencies
field.Simplifying this code makes it easier for us to add new features like an
!
ignore syntax to thedependencies
field.