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

fix: files not linting #320

Merged
merged 1 commit into from
Nov 26, 2023
Merged

fix: files not linting #320

merged 1 commit into from
Nov 26, 2023

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Nov 7, 2023

Fix files specified on the command line not linting as expected due to the generated patterns not working as intended if relative path that contains a file and not a directory due to an issue with CodeNarc pattern processing.

Fix incompatible arguments passed to CodeNarc by:

  • Using arrays internally to avoid issues with spaces in arguments being interpreted incorrectly.
  • Stripping all quotes from string arguments as CodeNarc doesn't handle them correctly.
  • Enable automatic argument quoting on Windows.

Fix command line -ext extensions not being processed correctly and matching too many files as it was missing the prefix.

Ensure readFile and writeFile calls produce a stack trace on failure due to: nodejs/node#30944.

Fix file delete race condition and variable clean up due to missing await.

Fix use of includes instead of exclude parameters.

Also:

  • Fixed Request failed logging
  • Fix README.md typo
  • Add additional useful debug logging
  • Run dev:pre-commit to update CHANGELOG.md
  • Add more cspell entries
  • Add missing items to CodeNarcServer.groovy usage
  • Re-enable tests which are now fixed
  • Override axios for security patch

@stevenh stevenh force-pushed the fix/file-patterns branch 3 times, most recently from b18216e to 8407ce2 Compare November 7, 2023 14:39
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 7, 2023

@nvuillam this is draft as the server is built against an unreleased version for CodeNarc including my PR to fix the pattern parsing and I need to address some niggles with files with spaces.

Also might be an idea to enforce all status checks passing on the repo before PR's can be merged as doc generation hadn't been run, which should have been flagged by the lint workflow.

@stevenh stevenh force-pushed the fix/file-patterns branch 2 times, most recently from af9ed89 to 1dfb0f9 Compare November 7, 2023 21:06
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 23, 2023

This is blocked by CodeNarc/CodeNarc#755

@nvuillam nvuillam marked this pull request as ready for review November 23, 2023 13:03
@nvuillam nvuillam marked this pull request as draft November 23, 2023 13:03
@nvuillam
Copy link
Owner

oops ^^

@stevenh
Copy link
Collaborator Author

stevenh commented Nov 23, 2023

This has highlighted some tests which are passing when they shouldn't. There's some pathing issues and looks like variance between how args are handled on Windows and Linux, in particular with files that have spaces in.

So more work to do on this one as well as the fix to CodeNarc.

@stevenh
Copy link
Collaborator Author

stevenh commented Nov 24, 2023

This is getting close now, CodeNarc has fix for excludes, just need to fix the variance on Windows with --noserver

@stevenh
Copy link
Collaborator Author

stevenh commented Nov 25, 2023

The Windows issue is down the argument handling set by node-java-caller.

PR to add option to allow that to be fixed is here: nvuillam/node-java-caller#47, will update this PR with a full fix once that is merged and released.

@nvuillam
Copy link
Owner

@stevenh PR in node-java-caller merged , and new patch released ^^

@stevenh stevenh force-pushed the fix/file-patterns branch 2 times, most recently from 2c09822 to f5ae2f0 Compare November 25, 2023 20:26
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 25, 2023

All tests now passing on all platforms. Fix for CodeNarc Windows relative paths also merged.

As the ETA for a new release is Jan / Feb next year, I suggest we use CodeNarc built from master for now and update to a tagged version later.

@stevenh stevenh marked this pull request as ready for review November 25, 2023 20:48
@nvuillam
Copy link
Owner

@stevenh I agree, if you are confident with master version of CodeNarc ( I know it is maintained seriously), I can merge this PR and release a new major version ^^ )

Just give me your go :)

Fix files specified on the command line not linting as expected due to
the generated patterns not working as intended if relative path that
contains a file and not a directory due to an issue with CodeNarc
pattern processing.

Fix incompatible arguments passed to CodeNarc by:
* Using arrays internally to avoid issues with spaces in arguments being
  interpreted incorrectly.
* Stripping all quotes from string arguments as CodeNarc doesn't handle
  them correctly.
* Enable automatic argument quoting on Windows.

Fix command line -ext extensions not being processed correctly and
matching too many files as it was missing the prefix.

Ensure readFile and writeFile calls produce a stack trace on failure
due to: nodejs/node#30944.

Fix file delete race condition and variable clean up due to missing
await.

Fix use of includes instead of exclude parameters.

Also:
* Fixed Request failed logging
* Fix README.md typo
* Add additional useful debug logging
* Run dev:pre-commit to update CHANGELOG.md
* Add more cspell entries
* Add missing items to CodeNarcServer.groovy usage
* Re-enable tests which are now fixed
* Override axios for security patch
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 26, 2023

Rebased to pickup new changes, should be good to go assuming tests all pass.

@nvuillam
Copy link
Owner

@stevenh please check your linkedin :)

Copy link
Owner

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, as usual , many thanks @stevenh :)

@nvuillam nvuillam merged commit f2f7cb0 into nvuillam:main Nov 26, 2023
6 checks passed
@stevenh stevenh deleted the fix/file-patterns branch November 29, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants