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

[internal] Parse addresses using a generated parser #14346

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Feb 3, 2022

To prepare to make the address syntax more complicated as part of #13882, move to parsing addresses using a generated peg parser (using the same parser crate that @jsirois introduced in #11922).

Additionally, simplify Address.spec and Address.path_safe_spec slightly by removing non-trivial early returns and reducing the total number of f-strings.

[ci skip-build-wheels]

[ci skip-build-wheels]

[ci skip-rust]
Comment on lines +34 to +35
peg::parser! {
grammar relative_address_parser() for str {
Copy link
Member Author

Choose a reason for hiding this comment

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

I was really impressed with how easy this crate was to use: good choice @jsirois!

Copy link
Member

Choose a reason for hiding this comment

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

I love PEG's. (first and only placed I've used/seen them is in Lua, which I think is where they originated, from Mr Roberto I)

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Neat!

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This is awesome!

Did you consider alternative locations for the Rust address crate? Because it is not useable by any other Rust crates other than our externs stuff, it feels like it's better colocated with that all, similar to how we have fs.rs etc.

@stuhood
Copy link
Member Author

stuhood commented Feb 3, 2022

Did you consider alternative locations for the Rust address crate? Because it is not useable by any other Rust crates other than our externs stuff, it feels like it's better colocated with that all, similar to how we have fs.rs etc.

Briefly... but I've been trying to bias away from adding more code to engine that can reasonable be separated. Unlike other ecosystems, the unit of compilation in rust is "the whole crate" (even with some amount of internal incremental compilation).

@stuhood stuhood merged commit 397e685 into pantsbuild:main Feb 3, 2022
@stuhood stuhood deleted the stuhood/address-parser branch February 3, 2022 16:46
stuhood pushed a commit that referenced this pull request Feb 17, 2022
alonsodomin pushed a commit to alonsodomin/pants that referenced this pull request Feb 25, 2022
Eric-Arellano pushed a commit to Eric-Arellano/pants that referenced this pull request Mar 3, 2022
…antsbuild#14485)

This issue was introduced by pantsbuild#14346 

[ci skip-rust]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Mar 3, 2022
…herry-pick of #14485) (#14696)

This issue was introduced by #14346 

[ci skip-rust]
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Mar 30, 2022
 #14347 added support for `Address` parameters, which are used via the `parametrize` builtin. But we had not yet added support for parsing parametrized addresses to CLI specs parsing.

This change adds support for parsing parametrized addresses in CLI specs by generalizing the parser that was added in #14346 to support parsing the additional syntax required for CLI specs (`!` ignores and `:`/`::` wildcards), and then using the same parser (with different validation) for both `Address` and `Specs` parsing.

Fixes #14521.

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this pull request Mar 30, 2022
…sbuild#14949)

 pantsbuild#14347 added support for `Address` parameters, which are used via the `parametrize` builtin. But we had not yet added support for parsing parametrized addresses to CLI specs parsing.

This change adds support for parsing parametrized addresses in CLI specs by generalizing the parser that was added in pantsbuild#14346 to support parsing the additional syntax required for CLI specs (`!` ignores and `:`/`::` wildcards), and then using the same parser (with different validation) for both `Address` and `Specs` parsing.

Fixes pantsbuild#14521.

[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Mar 30, 2022
…rypick of #14949) (#14957)

 #14347 added support for `Address` parameters, which are used via the `parametrize` builtin. But we had not yet added support for parsing parametrized addresses to CLI specs parsing.

This change adds support for parsing parametrized addresses in CLI specs by generalizing the parser that was added in #14346 to support parsing the additional syntax required for CLI specs (`!` ignores and `:`/`::` wildcards), and then using the same parser (with different validation) for both `Address` and `Specs` parsing.

Fixes #14521.

[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.

4 participants