Decoupling SettingsPanel from atom.config


#1

I’m looking at writing a package that provides a gui interface for viewing and modifying a project-level tsconfig.json file. This led me to look for a more general solution for editing any and all json configuration files, one that relies upon some kind of schema to prevent invalid values. When I read the documentation for the core Config class and realized that Atom already has some similar functionality in the form of package configurations, I thought I had found the perfect solution.

Not so much.

After reading through the CoffeeScript source for the SettingsPanel class, I realized that it was very tightly coupled to the atom.config object. Is there any reason why this could not or should not be changed? If the settings panel class was made available as a general solution for generating simple forms declaratively, then not only would my work be half done, it would open up a whole realm of possibilities for other json-based configuration files, including Atom package manifest files.

Thoughts?

P.S. First thoughts on CoffeeScript is that the syntax is a bit complicated in that there are so many different basic constructs. It’s like the evil twin of LISP. At this point, I am withholding further judgment, though.


#2

Could not? Not that I can think of, but there might be. Should not? Such an extensive rewrite could introduce a bunch of bugs and regressions. Additionally, since Atom only uses the SettingsPanel for working with atom.config, the change might create a maintenance burden that we don’t want to or have the resources to support.

But if there are no blockers and the change is made with care and quality and it is understood that adding features that aren’t applicable to the Settings View are out of scope, it could be a good thing.


#3

I think it could be accomplished pretty simply by adding a parameter to the constructor so a Config-shaped object can be passed into the constructor. You could even make the new parameter default to atom.config, so none of the existing call sites need to be changed. The chances of regression, in that case, would be minimal, but easily tested, no?


#4

Yes, I’ve read “Working Effectively with Legacy Code” too :wink: The chances of regression, whether minimal or not, are higher than not changing the code.

There are 16 instances of atom.config in lib/settings-panel.coffee:

34:        settings[name] = atom.config.get(name, scope: [@options.scopeName])
36:      settings = atom.config.get(namespace)
105:    params = {sources: [atom.config.getUserConfigPath()]}
107:    @disposables.add atom.config.observe(name, params, callback)
110:    params = {sources: [atom.config.getUserConfigPath()]}
113:    value = atom.config.get(name, params)
117:    params = {excludeSources: [atom.config.getUserConfigPath()]}
119:    atom.config.get(name, params)
124:        atom.config.unset(name, scopeSelector: @options.scopeName)
126:        atom.config.set(name, value, scopeSelector: @options.scopeName)
128:      atom.config.set(name, value)
201:  _.chain(settings).keys().sortBy((name) -> name).sortBy((name) -> atom.config.getSchema("#{namespace}.#{name}")?.order).value()
215:      schema = atom.config.getSchema("#{namespace}.#{name}")
230:  title = atom.config.getSchema(keyPath)?.title
237:  options = atom.config.getSchema(keyPath)?.enum ? []
313:  schema = atom.config.getSchema(keyPath)

This isn’t just deserializing some JSON and passing it into the constructor. You’ll have to create a mock object that mimics the behavior of the current Config class. So now we have a risk that the mock in settings-view will become out-of-sync with the actual Config class meaning that settings-view could be broken and the tests would never show it. Any time the behavior of Config changed, the settings-view package would potentially need to be updated.

There are other complications, but I’ll stop there. I honestly don’t think this will be as simple to do well as you believe it will be. I’m happy to be proven wrong though!