-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Project-specific Editor Settings #8101
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
base: master
Are you sure you want to change the base?
feat: Project-specific Editor Settings #8101
Conversation
4ian
left a comment
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.
Hi!
Thanks for opening this PR.
"ini" file format is a bit old. What do you think of using YAML instead? A few key things I see:
- It "looks good", can be written by a human.
- It's a superset of JSON.
- It is fairly popular in other tools (CI configuration files, etc...)
As for the implementation, my main thinking is:
- I think it's interesting :)
- We could even extend this to the web-app in the future by storing this somehow. (not mandatory for now)
- We should think about reducing the implementation by maybe having a whitelisted list of property names that can be overriden? I'll make a comment about this.
| settings: ProjectSettings, | ||
| preferences: Preferences | ||
| ): void => { | ||
| // At launch settings |
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 is fairly verbose. I wonder if we could reduce everything to a few key concepts:
- ProjectSettings renamed to "ProjectSpecificPreferences". It can be a "Partial" (or in Flow, I think it's
$Shape<Preferences>). - An "allowlist" of names of preferences that can be overwritten.
- When a project is loaded, preferences are loaded from it and applied without needing to code the calls to the setters.
This would reduce this function to something that iterates on the allowlist.
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.
The allowlist is basically what you called "VALID_PREFERENCE_KEYS"
| }); | ||
| }); | ||
|
|
||
| describe('VALID_PREFERENCE_KEYS', () => { |
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.
These tests are a bit of a "tautology". I would rather reduce the noise-to-signal ratio by not having them, as "testing a Set" is not super interesting and will actually make the codebase less flexible (because you have to remember updating these if at some point you rework the set to be something else). In other words, the Set is not really something I would expose to the "outside world".
I think the only test that would be worth having is one that the content from a file (Yaml file maybe, as discussed in my other comment) is properly applied to preferences.
Adds support for per-project editor preferences via
settings.inifile.Features
settings.inifrom project root when opening a projectininpm library for robust INI parsingUsage
Place a
settings.iniin the project root:Supported Settings
autosaveOnPreview,takeScreenshotOnPreview, etc.)use3DEditor,showBasicProfilingCounters)showExperimentalExtensions,showDeprecatedInstructionWarning, etc.)See
newIDE/docs/Project-Settings.mdfor the full list.