-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
.js.source
in config file gets preferredLineLength
and wrap-guide
added to it, which don't match my default preferences.
#697
Comments
As far as I know, and this applies to #696 as well, is Pulsar doesn't inject many values into a configuration file automatically. Generally the only items that will be added is those that are not the default values, as in only when you change a setting should new values be added. Is it possible there's another package activated causing this behavior? Could you maybe try listing your installed packages so we can see if one of them advertises this functionality? Or even better disable all packages, except core, remove these parts of your configuration then see if it occurs again, if it doesn't then it seems a community package is causing this behavior. |
@confused-Techie I started Pulsar in safe mode and verified the weird behavior before filing this issue. I thought safe mode kept it from loading any community packages. FWIW, the only community packages I have installed are |
Here's a more detailed steps-to-repro, which I think satisfies your concerns (and also covers the behavior observed in #696):
Here is the contents of the
A
The
|
@danfuzz Thanks a ton for providing such clear reproduceable steps here! I was able to reproduce the behavior described and was able to get to the bottom of it. This behavior stems from here where the atom.set("wrap-guide.columns", columns, {scopeSelector: `.${this.editor.getGrammar().scopeName}`}); Basically setting the That's why when you change the setting within Pulsar, if editing the file in Pulsar, you get a new I was able to track this file (across decaf, and repo migration) to the original PR that introduced this saving new settings behavior here, specifically this commit. Seems that this comment is what triggered this new behavior to be written. Which it's intention seems to make sense, that if the Since if the scoped settings are the default, we don't have to persist the changes to disk (I'd assume), since the scoped setting would then be used in the future. Admittedly, I'm not the most familiar with the full implications of handling scoped settings, and if there's a better way to accomplish this. @savetheclocktower Do you have a two cents to offer on this situation? Is there a better way to handle this situation? Is the current behavior necessary? Or is it something we shouldn't really worry about and just remove? Curious if you have any thoughts, otherwise I'd be inclined to think, that if someone has scoped settings different than other settings, that's their choice, and we shouldn't force any changes to propagate other scopes at all, as that's the users prerogative, and it's their complexity to deal with. Meaning I'd be on board with removing this behavior. But would appreciate any other thoughts on this one |
I can't dig into it until Monday, but if it can wait until then, I'm happy to offer advice. |
That sounds fine by me, whenever, if you get the chance I'd appreciate it. Otherwise like mentioned, my instinct says removing it is not a bad idea. |
I've read the original PR and I'm quite surprised that this sort of magical behavior was mooted without even any discussion about making this configurable. The idea that The quickest possible fix for this, and the one I'd be in favor of, is to put this behavior behind a More generally, I'm also surprised to see an instance in the codebase where a scope-specific setting is automatically applied in a certain scenario. There may well be other examples of it (because the APIs exist, and any package could do it), but I'm pretty sure that all of my own scope-specific config overrides have been manually added to |
@savetheclocktower I really appreciate you taking a look at this. But I'm totally on board with making this configurable, and just to keep expected support maybe we default to being enabled? Even if the idea seems a little outlandish. Otherwise I'm very happy to default to disabled from here on out. While leaving the future in, since you are right, sometimes it could make sense. I'd be happy to put together a PR for this here, unless you beat me too it (which feel free to do so), but otherwise just wanted to check in with your thoughts since you're a bit more familiar in this space, so I'm glad you had the time |
Defaulting to enabled is fine with me, and then if it bites other people in the future, we can decide if the default should be disabled instead. |
Sounds good to me then, appreciate your input |
Thanks in advance for your bug report!
What happened?
My default preference is to have an 80-column preferred line length, but I like being able to have wrap guides at a couple wider columns for the few cases where long lines are warranted. I tried to set this up in Pulsar, but when I do that Pulsar generates a couple properties under
.js.source
which don't match what I said. And really, I don't want Pulsar to insert those properties at all; my defaults under*
are in fact what I want for.js.source.
Pulsar version
1.108.0
Which OS does this happen on?
🍎 macOS
OS details
13.4
Which CPU architecture are you running this on?
Apple M1/M2
What steps are needed to reproduce this?
Pulsar > Config...
.*
:Result: The existing
.js.source
section will be edited to have:wrap-guide
, might as well make it what I want.".js.source > "wrap-guide"
so it reads:Result: The
preferredLineLength
under.js.source
gets changed to160
..js.source > editor > preferredLineLength
to be80
.Result: The
.js.source > "wrap-guide"
gets rewritten back to just[ 80 ]
.Additional Information:
No response
The text was updated successfully, but these errors were encountered: