-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Unify Settings
and AllSettings
#7532
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
c13f93f
to
acd6576
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
acd6576
to
22d29dd
Compare
22d29dd
to
530dba0
Compare
Self { | ||
cache_dir: cache_dir(&project_root), |
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.
Nit: maybe a comment on why these are separated from the rest? Same with other instantiations.
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.
I'll leave it as is because these will be moved to their own struct
in a PR higher in the stack, making the reason clear
530dba0
to
49afb83
Compare
Summary
This PR merges
AllSettings
andSettings
(by mergingCliSettings
intoSettings
). The main motivation is to avoid the confusion of why the two settings exist, which still isn't entirely clear to me.Here's what I understand today (I am happy to close this PR if we can get to the bottom of why the two structs exist)
#1891 split
Settings
intoAllSettings
andSettings
I solved this by extending the
CacheKey
derive macro to support thecache_key(ignore)
field attribute to ignore individual fields. We can keep the automatically derivedCacheKey
implementation and explicitly mark the excluded fields.There has been some argument whether the two settings exist because the
CliSettings
are only respected for the "root" configuration. This holds true for allCliSettings
exceptcache-dir
as #7520 showed. Meaning that distinction may have been true but no longer is.My final guess is that the
CliSettings
used to be unique to usingruff
from the cli. I think this is still mostly true (although most of the resolver code now lives insideruff_workspace
). However, I don't think it is sufficient reason to separate the two structs, especially considering that we could show diagnostics in our playground similar to biome (see console tab) where we would want to respect theoutput-format
configuration (and--show-sources
etc).fix
,fix-only
andcache-dir
likely remain CLI only settings but, IMO, don't justify the cognitive load of having multiple settings structs.The main justification for me to keep the separate settings struct is that the linter doesn't need the
CliSettings
. These are settings that only concern the diagnostic rendering, and file traversal (although the same is true forexclude
,respect_gitignore
and other global settings). That's why I plan to split settings in more semantic sub-settings:Test Plan
cargo test