Fix editorconfig parseSync option #387 - #394
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
- Coverage 99.68% 99.68% -0.01%
==========================================
Files 7 7
Lines 320 319 -1
Branches 90 95 +5
==========================================
- Hits 319 318 -1
Misses 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes the .editorconfig integration in Validator to address issue #387 by changing how the EditorConfig library is invoked so it no longer depends on process.cwd() being within the target project.
Changes:
- Add Node’s
pathmodule to support filename extraction. - Replace the previous CWD-based “relative path” logic with
path.basename(...)when passingconfigtoeditorconfig.parseSync().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7a9d837 to
c979a9f
Compare
|
Are we all good or is there anything else to clarify or modify ? |
|
Hi @Bartheleway, I am a bit busy and haven't really managed to have a deeper look yet. But I promise to do it soon. I am wondering at first sight if we still need all these path checks, or if we can simply forward the value into editorconfig... |
schorfES
left a comment
schorfES
left a comment
There was a problem hiding this comment.
Hi @Bartheleway,sorry for the long waiting...
From what I see now, I am pretty sure we can simplify a few things and adjust the tests accordingly. From my point of view, I would just pass a valid value for the editorconfig filename into the option and let editorconfig do the rest.
Thanks in advance for your collaboration, looking forward to your thoughts!
5b4dc12 to
a62a0ab
Compare
There was a problem hiding this comment.
Hi @Bartheleway, sorry for keeping you waiting so long for feedback. The changes look mostly good to me, except for the open issue flagged by Copilot.
Additionally, I don't see the need to update lint-staged and rename the config in this PR. Even so, that change is incomplete because the filename in .husky/pre-commit also needs to be adjusted. Regardless, I "cherry-picked" that change into #418.
Finally, please rebase your PR with the latest main to update lint-staged. You can then drop your 2nd commit.
We are almost at the finish line... 🏁
a62a0ab to
f824890
Compare
| ].forEach(editorconfig => { | ||
| const message = Messages.PATH_ISNT_FILE.message.replace('{a}', editorconfig); | ||
| const error = new Error(message); | ||
| it('should not throw if file does not exist', () => { |
There was a problem hiding this comment.
@Bartheleway Would you describe this change as a BREAKING CHANGE?
There was a problem hiding this comment.
Yes and no. Before we were expecting a filename or path and we now expect only a filename, this sounds like a breaking change. Giving a path didn't really work and that's why I did this PR so it is not really breaking but more making it to work 😄 (even though I am pretty sure I got it to work in some older version).
There was a problem hiding this comment.
But we're also not throwing any errors as we did before. So apart from the path vs. filename configuration that you mentioned, this also slightly changed the behavior. So I would tend to say we're going for a BREAKING CHANGE here and bump the version to 3.0.0.
Fix #387