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

Option to extend another unibeautify config #184

Open
muuvmuuv opened this issue Sep 20, 2018 · 11 comments
Open

Option to extend another unibeautify config #184

muuvmuuv opened this issue Sep 20, 2018 · 11 comments

Comments

@muuvmuuv
Copy link

muuvmuuv commented Sep 20, 2018

Some projects like webpack or TSLint have the ability to extend their config file. So we could have a different file in a subdirectory that extends or overrides parts of the one found in the root of a project.

Project
├── .unibeautifyrc.json
├── app
│   └── .unibeautifyrc.json
└── server
    ├── .eslintrc.json
    └── .unibeautifyrc.json

.unibeautifyrc.json @ root

{
  "*": {
    "indent_size": "2",
    "indent_style": "space"
  },
  "JSON": {
    "beautifiers": ["Pretty Diff"],
    "indent_size": "2",
    "indent_style": "space"
  }
}

.unibeautifyrc.json @ app

{
  "extend": "../.unibeautifyrc.json",
  "TypeScript": {
    "indent_style": "tabs"
  }
}

.unibeautifyrc.json @ server

EDIT: as @lassik said, it would be nice if it just looks a dir up when no file name is specified

{
  "extend": "..",
  "JavaScript": {
    "beautifiers": ["ESLint"],
    "ESLint": {
      "prefer_beautifier_config": true
    }
  }
}
@lassik
Copy link
Contributor

lassik commented Sep 20, 2018

As an alternative, .editorconfig does inheritance by always looking for .editorconfig files in all parent directories. The topmost file contains root = true to tell it to stop looking further.

I don't know whether the root or extend mechanism is a better fit for Unibeautify.

The TSLint config seems to have extends, so you can extend from multiple parents (multiple inheritance in a config system 😄). I assume they resolve conflicts by applying the configs in the order given and then the last definition wins.

@lassik
Copy link
Contributor

lassik commented Sep 20, 2018

IMHO, it would be nice if you could just give the directory (as in extend: "..") and it could automatically look for the default config file(s) there (.unibeautifyrc.json, etc.)

@muuvmuuv
Copy link
Author

@lassik extending would make it more flexible when projects want to use a config file from their own node package. e.g.:

"extend": "~additive-ci/.base-unibeautifyrc.json" (~ maybe indicator for nearest node_modules?)

@lassik
Copy link
Contributor

lassik commented Sep 20, 2018

I was under the impression that Unibeautify is supposed to be a long-lived project for universal use which just happens to be implemented in JS/Node. Therefore I would avoid baking in very Node-specific shortcuts. Also, there's a very long Unix tradition that ~ at the start of a pathname refers to a user home directory - another meaning would confuse people who are used to that.

Can the nearest node-modules be found easily via a relative pathname (i.e. is it usually at a fixed location relative to things in the repo)?

Another option (which is occasionally done elsewhere in Unix) is to have $FOO in a pathname expand to the value of the environment variable FOO, but that' s a bit sketchy since it's not easy for users to set up a consistent environment for all the programs that will read Unibeautify's rc file. We could also just have a facility for defining variables in the config files (Unibeautify rc files are based on CosmicConfig, does it support that out of the box?)

@muuvmuuv
Copy link
Author

muuvmuuv commented Sep 20, 2018

@lassik That is correct but node-sass is using ~ for the same approach. I was just copying the idea of having an indicator for the root of a project but it is true that it's not often a project (like when using an Editor). So forget this idea ^^

I think there is something like this: https://www.npmjs.com/package/find-node-modules or what do you mean?

I think it would be well enough to have it with a relative/absolute path to a file or a directory. Cosmiconfig can then take care of the rest.

So for example:

  • "extend": "../additive-ci/.base-unibeautifyrc.json"
  • "extend": ".."
  • "extend": "../../../conf/"
  • "extend": "/Users/marvinheilemann/Documents/Configs/.unibeautifyrc.json"
  • "extend": "/Users/marvinheilemann/Documents/Configs/"

@muuvmuuv
Copy link
Author

Additionally we could do something like:

"extend": [
   "/Users/marvinheilemann/Documents/Configs/.unibeautifyrc.json",
   "unibeautify:recommend",
   "unibeautify:prettier"
]

so users could use presets defined by the community? unibeautify:prettier would then use prettiers default config for all languages. This would also mean we need a for all langauge tag like * or _.

@Glavin001 what do you think about this?

@lassik lassik mentioned this issue Sep 20, 2018
@lassik
Copy link
Contributor

lassik commented Sep 20, 2018

I opened a separate issue about the presets. If we have a presets mechanism, exposing it via extend seems quite an elegant idea :)

IMHO, absolute pathnames like /Users/myname/... are mainly seen in repos by people who are new to text-based configuration frameworks, since the same absolute pathnames don't tend to work on different computers. But maybe there is some use case I haven't thought about (e.g. machine-generating Unibeautify rc files from a build script).

Sorry about all the nitpicking, I do like the main idea. Let's see what @Glavin001 has to say :)

@Glavin001
Copy link
Member

IMHO, absolute pathnames like /Users/myname/... are mainly seen in repos by people who are new to text-based configuration frameworks, since the same absolute pathnames don't tend to work on different computers.

💯% agree.

I was under the impression that Unibeautify is supposed to be a long-lived project for universal use which just happens to be implemented in JS/Node. Therefore I would avoid baking in very Node-specific shortcuts.

Also agree. Node.js is an implementation detail.

Also, there's a very long Unix tradition that ~ at the start of a pathname refers to a user home directory - another meaning would confuse people who are used to that.

I definitely do not want to have too much magic or unexpected behaviour. ~ indicates to me the Unix tradition, as I'm sure it will for many other users. Using another symbol may remove the Unix confusion, however, it's yet another non-standard thing for users to memorize.

"*": {
  "indent_size": "2",
  "indent_style": "space"
},

The * being apply to all? @szeck87 and I actually recently removed the global feature from Unibeautify in #62

I want users to be explicit when defining their Unibeautify configuration. For example, indent_size and indent_style are not valid options for the language Markdown: https://unibeautify.com/docs/language-markdown.html

Also, beautifiers is going to be required, as there will no longer be default beautifiers as there was with Atom-Beautify. Still remains a bug at https://github.com/Unibeautify/unibeautify/blob/master/src/beautifier.ts#L336
See Unibeautify/website#146 (comment) for recent problem @muuvmuuv and @szeck87 were discussing.

I also want to be careful when dealing with complex cases involving merging.
How should we merge languages? Merging beautifiers? Or other array type options?

EDIT: as @lassik said, it would be nice if it just looks a dir up when no file name is specified

{
 "extend": "..",

We can make this work. Currently we resolve the Unibeautify configuration given the directory of either the file being formatted or the project.


Lots to think about 🤔

@lassik
Copy link
Contributor

lassik commented Sep 21, 2018

Currently we resolve the Unibeautify configuration given the directory of either the file being formatted or the project.

Glad you brought this up :) This is an important point. A pathname given in a configuration file should always be resolved relative to the directory of that configuration file (except for some very exceptional cases where there is an obvious precedent to the contrary). I've come across at least a couple file formats where the designers neglected to take this into account, resolving pathnames relative to some non-obvious directory (in the worst case, the directory varies based on where the file is included from, or what tool or environment variables one happens to be using). As a user, it always results in metaphorical hair-pulling trying to figure the thing out.

If there's a need to specify pathnames relative to the project root, I would recommend an explicit syntax like ${projectroot}/path/to/file, while keeping each relative pathname path/to/file relative to the configuration file that contains it.

@lassik
Copy link
Contributor

lassik commented Sep 21, 2018

I also want to be careful when dealing with complex cases involving merging.
How should we merge languages? Merging beautifiers? Or other array type options?

Good point! It's possible we'll have to resort to specifying the merging rules on an option-by-option basis. But that's not too bad IMHO as long as the result is intuitive.

@Glavin001
Copy link
Member

A pathname given in a configuration file should always be resolved relative to the directory of that configuration file

💯% agree. I want the experience to be as intuitive as possible.

If there's a need to specify pathnames relative to the project root, I would recommend an explicit syntax like ${projectroot}/path/to/file, while keeping each relative pathname path/to/file relative to the configuration file that contains it.

Similar to how VSCode has some variables which are substituted: ${rootPath}, etc. See https://code.visualstudio.com/docs/getstarted/settings

It's possible we'll have to resort to specifying the merging rules on an option-by-option basis.

We may have to compare the advantages and disadvantages of a few different options. I'd like to look at how TSLint, ESLint, Prettier, etc handle these things and why.
I'm currently working on some other Unibeautify related tasks. Feel free to collect your ideas and feedback here, and I'll try to review ASAP. I think this is going to be a very important feature going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants