Skip to content
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

Project-specific config settings #1146

Open
1 task done
redyoshi49q opened this issue Nov 29, 2024 · 7 comments
Open
1 task done

Project-specific config settings #1146

redyoshi49q opened this issue Nov 29, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@redyoshi49q
Copy link

redyoshi49q commented Nov 29, 2024

Have you checked for existing feature requests?

  • Completed

Summary

I think it would be useful for the Pulsar editor to allow for project-specific overrides to be made to config.cson configuration values. This might be achievable by consulting a .pulsarrc file in the project's directory (which Google's search AI has "helpfully" hallucinated as a feature that Pulsar already supports), but an existing file/directory that's already used for project metadata might be a more appropriate (and probably more secure) place to implement such a thing instead.

What benefits does this feature provide?

This feature would allow for conveniently using settings that would benefit a particular project, even if such settings might not be desirable for Pulsar use in general.

As an example of how this could be useful, I've recently started looking into the git repository of a project that uses .html files for HTML content with Twig syntax (rather than another extension such as .html.twig). I have the php-twig package installed (and therefore have access to the "HTML (Twig)" grammar), but as far as I can tell, with the way things are now, I have a few broad options for how I use Pulsar, and none of them are entirely ideal:

  • I could leave my config.cson as it is; if I did, then I'd have to manually specify a HTML (Twig) grammar any time I opened an .html file in this project in order to functional syntax highlighting, and the project has a sizable number of these .html files.
  • I could specify in my config.cson file that .html files opened in Pulsar should use the HTML (Twig) grammar by default; doing this would let me be able to use HTML (Twig) syntax highlighting automatically for this project, but the setting change might negatively impact things for .html files opened for other projects (such as ones that use some other variation on standard HTML).
  • I could keep on editing my config.cson back and forth between the two prior options. While this does technically does allow for .html files to be given appropriate syntax highlighting in both the given project and in others, it's also a rather obnoxious way to achieve that result.

Having a place to specify per-project configuration settings would allow for avoiding the downsides that each of the currently available options would entail.

Any alternatives?

I'm not familiar enough with the nature of the API that packages have access to to assess whether this could potentially be implemented as a package. Pulsar uses packages for surprisingly low level features (such as tabs), but changes to where and how Pulsar derives its configuration settings might require an even higher level of access than what the packages API gives.

Other examples:

No response

@redyoshi49q redyoshi49q added the enhancement New feature or request label Nov 29, 2024
@savetheclocktower
Copy link
Contributor

There is an API for this, but it doesn't define any conventions about where the project-specific settings live. Two different packages use this API: project-config and atomic-management.

Both expect the custom config file to live in an .atom directory at the project root; the main difference between them is that project-config expects JSON and atomic-management expects CSON.

So that's the good news — you can install one of these packages and enjoy project-specific config settings today!

I expect that, if we ever did move this functionality into core, we'd “pave the cowpaths” and implement what already exists in these packages — likely with support for either file format.

@redyoshi49q
Copy link
Author

I have set up atomic-management in my own environment, and it seems to accomplish exactly what I was looking for. Thanks for the tip (and I hope that this bug report guides the next person who goes looking for this functionality).

I have a mild concern regarding security for this style of implementation (in that a .atom directory with malicious or short sighted contents might find its way into a git repository, potentially resulting in unanticipated behavior in Pulsar clients that download and open such a project), and if the functionality were to become core, I feel like that kind of possibility should be mitigated against in some fashion.

In any case, my own use case has been adequately handled; feel free to either close or leave open this issue as per your own preference.

@savetheclocktower
Copy link
Contributor

I have a mild concern regarding security for this style of implementation (in that a .atom directory with malicious or short sighted contents might find its way into a git repository, potentially resulting in unanticipated behavior in Pulsar clients that download and open such a project), and if the functionality were to become core, I feel like that kind of possibility should be mitigated against in some fashion.

If we did implement it in core, I imagine that you'd have to grant permission the first time you opened a project at a specific location. But I also think that there aren't major security implications here — in the sense that the worst it could do is change some of your settings. I can't immediately think of any packages that would do something silly like execute arbitrary code that happened to use a config setting as its payload.

The biggest issue, actually, may be the UX around custom settings, since they aren't visible in the settings-view package, nor can you modify them by changing settings in the settings-view GUI. One of the things I did last year was at least make it clear in settings-view when a setting has an active project-specific override — we include explanatory text for each such field in the GUI so that users aren't mystified why the changes they're making in that GUI don't seem like they're being applied.

@savetheclocktower savetheclocktower changed the title [feature request] Project-Specific Config Settings Project-specific config settings Nov 29, 2024
@savetheclocktower
Copy link
Contributor

I'm going to treat this as the canonical ticket for this request. That means it may be open for a while, but it'll also accumulate discussion and knowledge — some of which I'm about to dump below.

@savetheclocktower
Copy link
Contributor

History of project-specific config in Atom/Pulsar

Atom's support for project-specific config is minimal; it exists as two methods on atom.project. We're lucky, though, because this is the part that would've been very hard to implement by a community package.

RFC

RFC 001 proposed a project-specific config system.

  • It suggested a canonical project-specific config file name (hereafter referred to as “project config”) of atom-config.cson.
  • It envisioned that a project-specific setting could be set by passing { project: true } as part of the options argument to atom.config.set.

Near as I can tell, none of this ever got implemented! It may have clarified the discussion, but it wasn’t an influence on the API design.

Pull request

Separately, another user (apparently a contributor to Nuclide) opened a PR whose goals overlapped with those of the above RFC:

  • It envisioned a project config file named like [something].atomproject.{json,cson}.
  • In addition to the ability to layer project-specific config onto the window, it also imagined that the config file could include a list of paths that would automatically be opened when Atom was launched with a -p/--project parameter that pointed to the specific config file.

Over the course of the review of the aforementioned PR, the feature appeared to get progressively genericized. For instance: there doesn’t seem to be any code that enforces the [something].atomproject.{json,cson} naming convention; it was apparently assumed that you’d always specify the file via the -p parameter rather than have Atom look for it.

The most important part of the PR was the atom.project.replace API — though underdocumented, it’s what allows a specific project to include arbitrary configuration.

The ability to specify a project file via -p/--project was landed, but that parameter was removed only a month later; it apparently had never worked right. So atom.project.replace (and atom.project.onDidReplace) are the only evidence that this feature was added.

This, however, was enough — it was a feature without a GUI and without a canonical configuration location, but community packages rushed to fill the void.

Community usage

atomic-management and project-config work nearly identically: they both envision a config.{cson,json} file living within an .atom directory at your project root. File format is the only difference between them; atomic-management envisions a CSON file, whereas project-config envisions a JSON file.

This is a strong convention to build upon because it matches what VS Code ended up doing for a similar feature: putting all VS Code–specific stuff under a .vscode folder at the project root.

Though it’s not documented, both packages seem to allow you to include or eschew the top level of the config.cson schema. In other words: if you look at your own global config file, you’ll see that the top level contains a * key and may contain other keys whose values are scopes; this allows a user to define truly global config values, but also scope-specific overrides.

If you structure your project-specific configuration file the same way, it appears to work just fine. If you assume that all of your project-specific configuration overrides are applied to all scopes — and thus skip the top level of keys, much like the project-config README demonstrates — it also appears to work fine, and treats them as if you’d specified the overrides as the value of a * key.

My recollection is that this is a feature of the underlying atom.project.replace API.

Caveats

It’s fair to say that this feature was added without a huge amount of rigor; this was in 2018 at a point when (I suspect) Atom was clearly deprioritized after the Microsoft acquisition and the Atom team were looking out the window for their next opportunity. If there had been more attention paid to it, I would imagine that they’d have adopted this feature more holistically.

settings-view

For instance: settings-view had no concept of project-specific overrides. If you tried to view a setting that had a project-specific override, settings-view would happily display the override, but without explaining where the override came from. If you tried to change the setting via the GUI, it would not change the override; it would instead change your global config file at ~/.pulsar/config.cson. You might then wonder why the change doesn’t seem to take effect.

I fixed this in #655 by ensuring that settings-view excluded the project file as a source when retrieving settings. The upside is that it will always reflect what is in ~/.pulsar/config.cson (or the default value for that setting in the config schema); the downside is that it won’t ever show the overridden value. I did, though, add a warning message that tells a user when a value is currently being overridden by a project-specific setting; the purpose is to explain to a user why any changes they make in the GUI might not be reflected.

It would be nice to represent all of this complexity cleanly in the settings-view GUI, but I’m not aware of any conventions for this sort of thing. We’re already outliers (compared to VS Code and all other editors I’ve used) in the friendliness of settings GUI; VS Code, for instance, largely expects you to configure this stuff via JSON and relies on autocompletion to guide you toward valid configuration values.

Naming convention

It’s also true that neither the [something].atomproject.{json,cson} convention nor the atom-config.cson convention was ever applied. In retrospect, this is lucky, because I think the .atom folder convention is a stronger one; it matches .vscode and opens the door to further Pulsar-specific project customizations (like recommended packages, as VS Code does).

File formats

Atom and Pulsar both strongly tilt toward using CSON for configuration, but they both seem to support the usage of JSON; I’ve never tried it, but it seems to be possible to define a .pulsar/config.json file and have it be used instead of .pulsar/config.cson.

Likewise, if Pulsar supported a project-specific configuration file, it should allow either file format instead of forcing the user to choose (as both community packages do).

Multiple project roots

The RFC points out one potential gotcha from looking for a config file in a specific location from the project root: technically, an Atom/Pulsar project can have more than one root folder! (I don’t know whose idea this was, but it’s the bane of my existence.)

  • project-config handles this by considering only the first path returned by atom.project.getPaths().

  • atomic-management seems to check at the root of each path returned by atom.project.getPaths(), then merge them together such that earlier configs take precedence over later configs. It also exposes a GUI in the status bar that, when clicked, shows each config file in a list and allows you to enable or disable it.

The atomic-management path is closer to what I’d want Pulsar to support; the best way out of the dilemma of multiple project paths is to let the user know about the complexity and have them resolve it.

When project paths change

The RFC asks what should happen to config overrides when a new root is added to the project, or a root is removed from a project.

  • project-config has no specs and I can’t immediately verify that it does the right thing here.

  • atomic-management both (a) listens to atom.project.onDidChangePaths and re-runs its config assembly logic in response; (b) correctly resets Pulsar’s configuration to its initial state whenever no project-specific overrides are found.

Because it has solved the multiple-roots problem, atomic-management has demonstrated that this question has a trivial answer.


Conclusion

If we were to implement this, how would it work? Here’s my suggestion:

  • Practically all of what atomic-management does is correct; if we were to support this convention in core, we should use atomic-management as a guide.

  • We should explicitly support JSON in override files just like project-config does.

  • There are quality-of-life improvements we could apply:

    • When someone first opens a project at a given location and it includes project-specific overrides, they should at least be told about it, perhaps via notification.
    • When said config file is changed by a means other than direct editing by the user — e.g., a git pull includes an update to .atom/config.cson — we may want to notify the user in this instance as well.
    • I’m not convinced it should be the default behavior, but it’d be fair to include a setting like Ask before applying new config that is paranoid about config overrides such as these (changes that may not have been initiated by the user themselves) and quarantines them until the user has had a chance to review them.
  • There isn’t an immediate solution to the settings-view problem. The RFC correctly points out that this may not even be a problem; like language-specific settings, it’s an advanced feature, and though an solution might emerge, it’s not a blocker for shipping the enhancement.

    • If there were a way to make changes in the settings-view GUI and have them written to a project config file instead of .pulsar/config.cson, it would have to be opted into by the user — perhaps with some sort of persistent position: fixed drop-down in the corner of the viewport that allowed a user to choose a source. Implementing such a control would be tricky because (a) again, multiple project config files could be active at once; (b) it would also affect which values we display, and we’d still want to distinguish overridden values that differ from global config values.

    • In short, this part makes my head hurt and can be decided later.

In my opinion, the existence of atomic-management lessens the urgency of this feature in the short term; since it makes the “correct” decisions in nearly all cases, the main benefit of bringing this feature into the core would be discoverability (in theory).

It would still be worth bringing into core in the future if we did want to go further and support more project-specific stuff in the style of VS Code.

@savetheclocktower
Copy link
Contributor

There's one other issue that's a bit deeper and I'm not sure what the resolution should be:

Suppose you have a config key whose value is an array, as in the example in #655’s description. If you wanted to override the config value merely to add a value that may not be there, you're stuck; you can only replace the value altogether. Likewise, you can't subtract a value that may be present. To apply the modification you want, you have to know ahead of time what the current value is, and that's not realistic if a team is sharing an .atom/config.cson file.

This is a result of atom.project.replace inheriting basic Object.assign semantics. There are ways of applying patches to JSON structures, but they tend to be either incredibly complicated (like the JSON Patch standard described by RFC 6902) or just somewhat complicated (like the JSON Merge Patch standard described by RFC 7386); and the latter standard doesn't even support the array modifications that I just described.

Such enhancements might be worth applying in the future — perhaps as a new file inside the .atom directory so that we don't have to complicate the format of .config.cson. This sort of thing is also easily done with arbitrary code, but adding support for something like .atom/init.js is quite fraught, and I'm not sure it's a good idea (even if we sandbox it and make the user approve it first).

@redyoshi49q
Copy link
Author

Per-project array configuration might be tricky to do well. Suppose that there's a particular value within an array in the global config file; a session is then opened within that environment such that one project's config file adds that value in again, two more explicitly remove that same value, a fourth makes unrelated changes to the array storing that value, and a fifth doesn't change the array at all. It's not entirely clear from a technical standpoint whether the application should treat the configuration as having that value set or not, and if the config files themselves are ordered to give one of the project's config files priority over the others, that might interfere with the desired result for /other/ configuration values.

An implicit priority scheme (a la CSS) or an explicit priority value (a la Linux package management) might even be warranted to remove the ambiguity of how such a case should be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants