-
Notifications
You must be signed in to change notification settings - Fork 5
Non-Standard Key Support #24
Comments
Personally, I don't think support non-standard keys is a good idea. |
hey @Drakmyth, thanks for continuing this conversation! I appreciate all the links you've provided that indicate existing intention and implementations of parsing non-standard/custom properties. It seems like this question might not be as simple as I thought it was previously. @fisker, thanks for chiming in as well! As you (@fisker) mentioned in #16 (comment), this package was originally intended for use by Prettier, and while there are some other dependents (according to github and npm), Prettier is still the vast majority of the usage this package sees. Given that, I think it might make sense for this package's code/functionality to move into the Prettier repository, so that any issues relating to it can be discussed where it's used. Personally, I don't use What do y'all think of that proposal? If we have https://github.com/prettier/prettier/ vendor this repository's code, then I can archive this repo and direct people to discuss functionality in the Prettier issues. |
Makes sense to me. When I started using Prettier, given EditorConfig support was called out in the docs I was expecting that support to be managed by https://github.com/prettier/prettier/. It was only while looking through that repo for other EC-related issues and discussions that I learned about this repository and that the maintainers over there seemed to largely be directing any EC-related issues here, hence my posting this issue here instead of there. If the Prettier application is already modifying the configuration this package provides, I question the benefit of this being separate. At the least it seems like it should be maintained by https://github.com/prettier, and the question of whether it should remain as a separate package/repository or be merged into the Prettier codebase completely can be answered separately once they're maintained by the same people. |
Thanks for the context, I definitely agree with you opening the issue here, given that this is where things are currently implemented 👍🏻
That's what I'm thinking, this is better suited to be maintained as part of the broader Prettier maintenance effort, rather than being off to the side in its own repo. @fisker is the primary committer to Prettier currently, let's see what they think! |
Make sense to me, we can move the code to Prettier I think. Unless @sosukesuzuki have a different opinion. |
Thanks @fisker! As you noticed, I'm working on this in prettier/prettier#15394. Once that's merged, I'll plan to close this issue (and #16) and ask @Drakmyth to re-open it in the Prettier repo |
I'd like to vendor/upstream this code because it's been the topic of a handful of issues in its repo over the years, including josephfrazier/editorconfig-to-prettier#9, josephfrazier/editorconfig-to-prettier#21, and most recently josephfrazier/editorconfig-to-prettier#24, and I feel that having it in-repo would be more appropriate (not to mention [the issues in _this_ repo that pertain to editorconfig parsing](https://github.com/search?q=repo%3Aprettier%2Fprettier+editorconfig++&type=issues&state=open)). I've elaborated on my thoughts in josephfrazier/editorconfig-to-prettier#24 (comment), which I'll quote below: > hey @Drakmyth, thanks for continuing this conversation! I appreciate all the links you've provided that indicate existing intention and implementations of parsing non-standard/custom properties. It seems like this question might not be as simple as I thought it was previously. > > @fisker, thanks for chiming in as well! > > As you (@fisker) mentioned in [#16 (comment)](josephfrazier/editorconfig-to-prettier#16 (comment)), this package was originally intended for use by Prettier, and while there are some other dependents (according to [github](https://github.com/josephfrazier/editorconfig-to-prettier/network/dependents?dependent_type=PACKAGE) and [npm](https://www.npmjs.com/package/editorconfig-to-prettier?activeTab=dependents)), Prettier is still the vast majority of the usage this package sees. > > Given that, I think it might make sense for this package's code/functionality to move into the Prettier repository, so that any issues relating to it can be discussed where it's used. Personally, I don't use `.editorconfig` with Prettier, so I don't feel especially well-positioned to make decisions on how things should work, especially when [the output of this package is subsequently modified by Prettier](https://github.com/prettier/prettier/blob/7c512651f8814924a042034ff821190c68ef409d/src/config/resolve-editorconfig.js#L13-L16) (I think you had also [mentioned](#14514 (comment)) this previously, @fisker), indicating that its behavior is already divergent from the intended use case. > > What do y'all think of that proposal? If we have https://github.com/prettier/prettier/ vendor this repository's code, then I can archive this repo and direct people to discuss functionality in the Prettier issues. This also implements @fisker's suggested change to ES module syntax from josephfrazier/editorconfig-to-prettier#16, as it's required for lint/test etc to pass. Once this is merged, I'll likely archive the https://github.com/josephfrazier/editorconfig-to-prettier/ repo after adding a note directing any visitors here instead. --- * Vendor editorconfig-to-prettier.js from https://github.com/josephfrazier/editorconfig-to-prettier/blob/8bb8226c40d7f2c8cf629301576850876c8617d4/index.js * WIP Use `require` instead of `import` * Revert "WIP Use `require` instead of `import`" This reverts commit 4656b72. * Use `export` in editorconfig-to-prettier.js instead of `module.exports`, see josephfrazier/editorconfig-to-prettier#16 Copied from https://raw.githubusercontent.com/josephfrazier/editorconfig-to-prettier/e245d7051f3e2a1153b82362ed241f3f7482ab18/index.js * `yarn fix` * Add tests/unit/editorconfig-to-prettier.js from josephfrazier/editorconfig-to-prettier#16 Copied from https://raw.githubusercontent.com/josephfrazier/editorconfig-to-prettier/9effd580e7b6722007e1ace9913512e450cda46f/test.js * tests/unit/editorconfig-to-prettier.js: Fix path to implementation * WIP Naively convert assert.deepEqual to expect/toStrictEqual, see #15394 (comment) * Wrap `test()` around `expect` calls * XXX Force a middle test to fail to ensure the syntax works * Revert "XXX Force a middle test to fail to ensure the syntax works" This reverts commit 947eb34. * `yarn fix`
Ok @Drakmyth , prettier/prettier#15394 was merged, so if you would open this issue in the prettier repo, the conversation can continue there |
Hi. I'm a newcomer to both Prettier and EditorConfig but am liking what I'm seeing with both of them. I just wanted to throw another vote out for being able to support keys not defined in the EC spec from the
.editorconfig
file. I saw (#9) and (#21) were already closed on this topic, but I wanted to provide some additional points in support I came across and wasn't sure if an already closed ticket was an appropriate place to continue the discussion.Reading editorconfig/editorconfig#415 and editorconfig/editorconfig#60 (comment) it looks as though the expectation from the EditorConfig team is that tools would be able to read domain-specific or even tool-specific properties from the
.editorconfig
file that aren't necessarily defined by the EditorConfig spec.It also looks like ReSharper (and other JetBrains tools) and Roslyn both heavily support custom keys for configuration.
My use case is currently very small - the only non-standard key I wish to configure right now is the
bracketSameLine
option - but it's annoying that I either need to have two separate config files (.editorconfig
and.prettierrc
) or have to move away from EditorConfig entirely to do so.Not sure if this moves the needle on adding support for this or not. I'm nearly always on the train of not adding custom extensions for things so while my initial thought was the same as expressed in (#9), seeing that the EditorConfig team is actually encouraging the practice changed my mind and seeing these issues I wanted to make sure others were aware too.
The text was updated successfully, but these errors were encountered: