Clean up the config parsing code. #12678

benjyw · 2021-08-27T16:56:36Z

The config parsing code had some historic idiosyncrasies, unnecessary bloat and complexity that were easy to strip out.

Config used to have two load methods, one for loading from a FileContent, another for loading directly from files. And those methods called an underlying method awkwardly, by passing an "opener" in, which was clunky. Since the file method was only used in tests, so this change gets rid of it and switches everything to working with FileContent.
There is a weird complication where during bootstrapping we can't use a real FileContent, because of cyclic dependencies, so we create an identical dataclass for use just in bootstrapping, and rely on duck typing. This change introduces a Protocol to formalize that duck typing and have it statically checked.
Gets rid of _EmptyConfig, which is both unnecessary (a _ChainedConfig with no underlying configs has the desired behavior), and technically incorrect (it would return None, instead of erroring, if you tried to get a value).
Fixes some type signatures to refer to the public class Config, and not the specific subclasses, which are not of interest.
A few other small simplifications.

This change also:

Moves GlobMatchErrorBehavior to its own source file, to avoid a dependency cycle. Previously we were re-exporting it from pants.engine.fs, which was overkill for a trivial enum.
Renames is_valid_scope_name_component to is_valid_scope_name, since we no longer have scope names consisting of dotted components.

This is prep work to make it easier to support TOML nested tables properly, once we deprecate the one remaining dotted options scope.

[ci skip-rust]

[ci skip-build-wheels]

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

benjyw · 2021-08-27T17:05:08Z

src/python/pants/option/config.py

        if not self.has_option(section, option):
            return default

        raw_value = self.get_value(section, option)
-        if type_ == str or issubclass(type_, str):
+        if issubclass(type_, str):


issubclass(str, str) is True so no need for the first check.

benjyw · 2021-08-27T17:08:09Z

src/python/pants/option/config.py

+                config_values = cls._parse_toml(
+                    file_content.content.decode(), normalized_seed_values
+                )
+            except Exception as e:


Distinguishing the two cases seemed unnecessary: if a file fails to parse, the fact that it did or didn't have a .toml suffix is immaterial.

stuhood · 2021-08-27T17:24:19Z

As mentioned in Slack, we really need to be looking for opportunities to migrate some of this to consuming the rust code that we have for this: https://github.com/pantsbuild/pants/blob/main/src/rust/engine/client/src/options/config.rs ... cleanups are fine, but if we're going to be adding features, we'd technically need to be adding them in two places unless we migrate.

benjyw · 2021-08-27T17:30:38Z

As mentioned in Slack, we really need to be looking for opportunities to migrate some of this to consuming the rust code that we have for this: https://github.com/pantsbuild/pants/blob/main/src/rust/engine/client/src/options/config.rs ... cleanups are fine, but if we're going to be adding features, we'd technically need to be adding them in two places unless we migrate.

This doesn't add any features AFAICT.

stuhood · 2021-08-27T17:40:46Z

This doesn't add any features AFAICT.

Right, but it says:

This is prep work to make it easier to support TOML nested tables properly, once we deprecate the one remaining dotted options scope.

... so I just wanted to pop in and remind that an alternative to cleaning up this code would be to replace it. Not blocking.

Eric-Arellano · 2021-08-27T17:49:34Z

Moves GlobMatchErrorBehavior to its own source file, to avoid a dependency cycle. Previously we were re-exporting it from pants.engine.fs, which was overkill for a trivial enum.

I disagree. I think it's important to keep re-exporting it so that plugin authors have all their filesystem-related imports from pants.engine.fs, which also faciliates discovery. It was very intentional to do this. And no longer re-exporting breaks callsites if they use MyPy.

We re-export with several other types so that pants.engine is the main entry point for plugin authors, e.g. pants.engine.addresses re-exporting from pants.base.address. Or pants.engine.rules re-exporting pants.engine.internal.selectors.

It's fine to move the original definition, but please change back to re-export.

Eric-Arellano · 2021-08-27T17:50:42Z

so I just wanted to pop in and remind that an alternative to cleaning up this code would be to replace it. Not blocking.

I think there's still value in landing this: it will make the Rust port easier because the original code is simpler to understand. I'm excited about this change!

# 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]

benjyw · 2021-08-29T08:15:18Z

Moves GlobMatchErrorBehavior to its own source file, to avoid a dependency cycle. Previously we were re-exporting it from pants.engine.fs, which was overkill for a trivial enum.

I disagree. I think it's important to keep re-exporting it so that plugin authors have all their filesystem-related imports from pants.engine.fs, which also faciliates discovery. It was very intentional to do this. And no longer re-exporting breaks callsites if they use MyPy.

We re-export with several other types so that pants.engine is the main entry point for plugin authors, e.g. pants.engine.addresses re-exporting from pants.base.address. Or pants.engine.rules re-exporting pants.engine.internal.selectors.

It's fine to move the original definition, but please change back to re-export.

Restored, and commented so this doesn't happen again. In future, let's be sure to document this sort of non-obvious thing.

benjyw · 2021-08-29T22:04:52Z

Are the bandit tests known to be flaky? They pass locally on my laptop, but fail consistently in CI, and this seems unrelated to my change.

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

benjyw · 2021-08-29T22:46:18Z

Looks like merging with latest master fixed the issue... Thanks for the review!

* Clean up the config parsing code. ([pantsbuild#12678](pantsbuild#12678)) * [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([pantsbuild#12692](pantsbuild#12692)) * [internal] Add missing `resources` targets for default tool lockfiles ([pantsbuild#12670](pantsbuild#12670)) * [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([pantsbuild#12669](pantsbuild#12669)) * [internal] Test that default tool lockfiles work on macOS ([pantsbuild#12666](pantsbuild#12666)) * [internal] Improve the error message for stale/invalid lockfiles ([pantsbuild#12618](pantsbuild#12618)) * [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([pantsbuild#12641](pantsbuild#12641)) * [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([pantsbuild#12665](pantsbuild#12665)) * [internal] Test that Python tools work with all expected interpreter versions ([pantsbuild#12656](pantsbuild#12656)) * [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([pantsbuild#12630](pantsbuild#12630)) * [internal] Add links to changelog and Twitter in Pants PyPI project page. ([pantsbuild#12636](pantsbuild#12636)) * [internal] Update CoC with proper markdown + https links. ([pantsbuild#12637](pantsbuild#12637)) * GitHub issue templates ([pantsbuild#12617](pantsbuild#12617)) * Register subsystems and target types by backend/plugin. ([pantsbuild#12623](pantsbuild#12623)) * [internal] Sort input requirements for `LockfileMetadata` ([pantsbuild#12615](pantsbuild#12615)) * [internal] Expect lockfile metadata to be defined ([pantsbuild#12616](pantsbuild#12616)) * Simplify subsystem registration. ([pantsbuild#12620](pantsbuild#12620)) * [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([pantsbuild#12613](pantsbuild#12613)) * [internal] Refactor `lockfile_metadata.py` to be more class-based ([pantsbuild#12611](pantsbuild#12611)) * Embrace that help strings are markdown. ([pantsbuild#12606](pantsbuild#12606)) * Stop fetching a lockfile request just to figure out the requirements digest ([pantsbuild#12607](pantsbuild#12607)) * [internal] Refactor MyPy first party plugins ([pantsbuild#12599](pantsbuild#12599)) * [internal] Refactor MyPy config file setup ([pantsbuild#12593](pantsbuild#12593)) * [internal] use `is_union()` rather than `union.is_instance()`. ([pantsbuild#12595](pantsbuild#12595)) * fs_util can use mTLS and ignore certificates ([pantsbuild#12586](pantsbuild#12586)) * [internal] Fix Pylint lockfile to actually be wired up ([pantsbuild#12590](pantsbuild#12590)) * [internal] Update lockfile TODOs with current state of project ([pantsbuild#12592](pantsbuild#12592)) * [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints ([pantsbuild#12566](pantsbuild#12566)) * [internal] Refactor Pylint first-party plugins ([pantsbuild#12583](pantsbuild#12583)) * [internal] Fix Pytest and IPython to check for stale lockfiles ([pantsbuild#12582](pantsbuild#12582)) * [internal] Do not cache lockfile generation ([pantsbuild#12674](pantsbuild#12674))

* Clean up the config parsing code. ([#12678](#12678)) * [internal] `--generate-lockfiles-resolve` does not determine `PythonLockfileRequest` for unspecified tools ([#12692](#12692)) * [internal] Add missing `resources` targets for default tool lockfiles ([#12670](#12670)) * [internal] Reorganize lockfile code to no longer be in `backend/python/experimental` ([#12669](#12669)) * [internal] Test that default tool lockfiles work on macOS ([#12666](#12666)) * [internal] Improve the error message for stale/invalid lockfiles ([#12618](#12618)) * [internal] Rename `./pants lock` to `./pants generate-user-lockfile` and `./pants tool-lock` to `./pants generate-lockfiles` ([#12641](#12641)) * [internal] Only run macOS tests in CI annotated with `@pytest.mark.platform_specific_behavior` ([#12665](#12665)) * [internal] Test that Python tools work with all expected interpreter versions ([#12656](#12656)) * [internal] Factor out a helper rule for setting up `--resolve-all-constraints` ([#12630](#12630)) * [internal] Add links to changelog and Twitter in Pants PyPI project page. ([#12636](#12636)) * [internal] Update CoC with proper markdown + https links. ([#12637](#12637)) * GitHub issue templates ([#12617](#12617)) * Register subsystems and target types by backend/plugin. ([#12623](#12623)) * [internal] Sort input requirements for `LockfileMetadata` ([#12615](#12615)) * [internal] Expect lockfile metadata to be defined ([#12616](#12616)) * Simplify subsystem registration. ([#12620](#12620)) * [internal] Use constants for magic strings `<none>` and `<default>` for tool lockfiles ([#12613](#12613)) * [internal] Refactor `lockfile_metadata.py` to be more class-based ([#12611](#12611)) * Embrace that help strings are markdown. ([#12606](#12606)) * Stop fetching a lockfile request just to figure out the requirements digest ([#12607](#12607)) * [internal] Refactor MyPy first party plugins ([#12599](#12599)) * [internal] Refactor MyPy config file setup ([#12593](#12593)) * [internal] use `is_union()` rather than `union.is_instance()`. ([#12595](#12595)) * fs_util can use mTLS and ignore certificates ([#12586](#12586)) * [internal] Fix Pylint lockfile to actually be wired up ([#12590](#12590)) * [internal] Update lockfile TODOs with current state of project ([#12592](#12592)) * [internal] Check for lockfile staleness by using context's interpreter constraints, rather than global constraints ([#12566](#12566)) * [internal] Refactor Pylint first-party plugins ([#12583](#12583)) * [internal] Fix Pytest and IPython to check for stale lockfiles ([#12582](#12582)) * [internal] Do not cache lockfile generation ([#12674](#12674))

CLean up the config parsing code.

f418e4e

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

benjyw commented Aug 27, 2021

View reviewed changes

benjyw requested a review from Eric-Arellano August 27, 2021 17:07

benjyw changed the title ~~CLean up the config parsing code.~~ Clean up the config parsing code. Aug 27, 2021

benjyw commented Aug 27, 2021

View reviewed changes

Restore re-export from pants.engine.fs

94043e1

# 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]

Eric-Arellano approved these changes Aug 29, 2021

View reviewed changes

Merge branch 'main' into cleanup_config_parsing

79f7856

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

benjyw merged commit 409e052 into pantsbuild:main Aug 29, 2021

benjyw deleted the cleanup_config_parsing branch August 29, 2021 22:47

benjyw restored the cleanup_config_parsing branch August 29, 2021 22:47

benjyw deleted the cleanup_config_parsing branch August 29, 2021 22:47

illicitonion mentioned this pull request Aug 30, 2021

Prep for 2.7.0rc0 #12695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up the config parsing code. #12678

Clean up the config parsing code. #12678

benjyw commented Aug 27, 2021 •

edited

Loading

benjyw Aug 27, 2021

benjyw Aug 27, 2021

stuhood commented Aug 27, 2021

benjyw commented Aug 27, 2021

stuhood commented Aug 27, 2021

Eric-Arellano commented Aug 27, 2021

Eric-Arellano commented Aug 27, 2021

benjyw commented Aug 29, 2021

benjyw commented Aug 29, 2021

benjyw commented Aug 29, 2021

Clean up the config parsing code. #12678

Clean up the config parsing code. #12678

Conversation

benjyw commented Aug 27, 2021 • edited Loading

benjyw Aug 27, 2021

Choose a reason for hiding this comment

benjyw Aug 27, 2021

Choose a reason for hiding this comment

stuhood commented Aug 27, 2021

benjyw commented Aug 27, 2021

stuhood commented Aug 27, 2021

Eric-Arellano commented Aug 27, 2021

Eric-Arellano commented Aug 27, 2021

benjyw commented Aug 29, 2021

benjyw commented Aug 29, 2021

benjyw commented Aug 29, 2021

benjyw commented Aug 27, 2021 •

edited

Loading