Repository navigation
Conversation
`dump-config` reports only the interpreter and the paths imports resolve from, so there is no way to see what any other setting resolved to. Someone debugging a config that appears to be ignored has nothing to inspect. This adds the end-to-end test for a `--full` flag that prints the rest of the resolved settings, ahead of implementing it. The config under test sets a path-scoped override, two error severities written out of alphabetical order, and a misspelled key, so the test pins down the sort order and the reporting of unrecognized keys as well as the overall shape of the output.
Users are pointed at `dump-config` to work out why a config is not behaving the way they expect, but it only reports the interpreter and the paths imports resolve from. Every other setting is invisible, and a key Pyrefly did not recognize is dropped silently, which is exactly the case the command is reached for. `--full` prints the rest of the resolved configuration in the indented, prose-labelled style the command already uses. Default output is untouched, so the existing behaviour and its tests still hold. The rendering lives in `pyrefly_config` next to `structured_import_lookup_path` because several `ConfigBase` settings are crate-private and unreadable from the command, which only prints the lines it is handed. Settings the import lookup path already covers are left out so that `--full` adds information rather than repeating it, and the error severities are sorted on the way out because they are stored in a `HashMap`.
The import resolution guide is where users are sent to debug configuration problems, so the new flag is described alongside the command it belongs to.
|
Hi @ravivats! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
This comment has been minimized.
This comment has been minimized.
|
I will send the updated code for it soon. |
Sorry, you can ignore that bot message! We're a bit behind on PR reviews right now, but I still intend to review this as soon as I can. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
rchen152
left a comment
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! Sorry for the delayed review. I'm concerned about the maintainability of base_settings, since it creates a duplicate list of all of our config settings, which we'd have to remember to update every time we add a new setting.
I think a more maintainable alternative would be to use the toml crate (https://docs.rs/toml/latest/toml/index.html) to convert the config to toml, remove all the keys that are already printed, and then print the rest.
|
This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks. If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over. Thank you for your contributions! |
|
Hey @ravivats! Just wanted to check if you're still interested in working on this PR. (No worries if not - I realize it took me quite a while to review!) |
|
I'm going to go ahead and close this PR, but feel free to open a new one if you'd like to continue working on this issue. |
Summary: Adds `dump-config --full` to show resolved configuration settings beyond the interpreter and import paths, making it easier to understand why a setting is not taking effect. Fixes #1483. This replaces #4732 and addresses rchen152’s review feedback. ## Approach The command serializes the resolved configuration using the existing `toml` crate, removes settings already covered by the interpreter and import-path output, and prints the remaining settings as TOML. The output includes error severity overrides, path-scoped `sub-config` overrides, and unrecognized configuration keys and their values. Tables are sorted recursively for stable output, while array order is preserved. Default command output and `--max-files` behavior remain unchanged. Unset options and some settings at their default values are omitted according to the existing serialization rules. ## Addressing the earlier review In [the review of https://github.com/meta/pyrefly/issues/4732](https://github.com/meta/pyrefly/pull/4732#pullrequestreview-5361524779), rchen152 pointed out that `base_settings` duplicated the list of configuration fields and would need updating whenever a setting was added. Following that suggestion, I replaced the manual field list and custom rendering with TOML serialization. New serializable settings are now included automatically, without maintaining a second list. ## Why `--full` instead of `--verbose`? The issue suggests `--verbose`, but the existing global `-v/--verbose` flag controls logging verbosity. `--full` keeps configuration output separate from logging. Pull Request resolved: #5473 Test Plan: - `cargo test -p pyrefly_config --lib --locked`: 271 tests passed. - Scrut tests for `test/config.md` and `test/interpreter.md`: 25 tests passed. - Formatting and Clippy checks passed, with only pre-existing warnings in untouched code. - Regression coverage includes nested error overrides, stable ordering, wildcard values, unrecognized keys, and command-line overrides. Validation ran on macOS. Linux and Windows validation remains for CI. ## AI usage disclosure I used Claude for parts of the original investigation and implementation, update the tests and documentation, and run validation. Reviewed By: rchen152 Differential Revision: D128841161 fbshipit-source-id: f3a71fa14e1855bfd7feaf0f86d3c1d7f0cff009
Summary
Print every resolved setting under dump-config --full
Fixes #1483
Problem
dump-configis what the docs point people at to debug configuration, but it reports only the interpreter and the paths imports resolve from. Every other resolved setting is invisible, and a config key Pyrefly did not recognize is dropped silently, which is exactly the case people reach for this command to diagnose.Approach
Adds
--full, which prints the rest of the resolved configuration in the same indented, prose-labelled style the command already uses. Default output is unchanged.As per the comment on the issue, the file-list output is untouched since
--max-filesalready covers it. Settings that already appear underResolving imports from:are deliberately omitted so that--fulladds information rather than repeating it.The rendering lives in
pyrefly_configbesidestructured_import_lookup_path, mirroring that pattern, because severalConfigBasesettings are crate-private and unreadable fromdump_config.rs. Error severities are sorted on the way outbecause
ErrorDisplayConfigwraps aHashMap.On the flag name (--full instead of --verbose as suggested in the issue description)
The issue says "when
--verbose", but there is already a global-v/--verboseon every subcommand (commands/util.rs) that raises log verbosity, and clap rejects a duplicate. I used--fullto keep config output independent of lognoise.
Testing
test/config.mdcargo test(8227 passed),cargo clippy,cargo fmtall cleandump-configgolden tests intest/config.mdandtest/interpreter.mdpass unchanged, confirming default outpAI usage disclosure
I have worked on this issue for things like setting up the
pyreflyrepository as well as understanding the code and current dump-config output etc. using Claude, but I have done partially the implementation by hand, and I have reviewed and verified the implementation, the tests, and edited this description.