search

nitely / nim-regex

Pure Nim regex engine. Guarantees linear time matching
https://nitely.github.io/nim-regex/
MIT License
227 stars 20 forks source link

Fix all styleCheck warnings and enforce styleCheck #40

Closed kaushalmodi closed 5 years ago

kaushalmodi commented 5 years ago

The warnings were found by running with --styleCheck:hint.

Fixes:

Note that the styleCheck enforcing will work only with Nim 0.20.0 and newer because the --styleCheck switch got introduced in that version.

The --styleCheck:error will result in errors on nim 0.20.0 and 0.20.2 due to styleCheck failures on few stdlibs. Those will be fixed on Nim devel soon. So I am enabling the --styleCheck:error switch only on nim versions newer than 0.20.2.

nitely commented 5 years ago

Hey, can we enforce this somehow?. I don't want PRs fixing "style"...

kaushalmodi commented 5 years ago

can we enforce this somehow?

To enforce it, you can add switch("styleCheck", "error") in a config.nims file in the base directory of this repo.

But even if you fix all those style issues, you will be unable to fix the reset/reSet error because of this bug: https://github.com/nim-lang/Nim/issues/11756.

A workaround might be to refactor the reSet enum value to something else.

I don't want PRs fixing "style"...

I can understand. You don't have to accept this PR as it's trivial. Feel free to commit these changes yourself. Once you create the above config.nims, you'll know what all needs to be fixed.

kaushalmodi commented 5 years ago

If you want to wait for the reset/reSet bug to get fixed or don't want to refactor reSet, you can put switch("styleCheck", "hint") instead in the config.nims.

kaushalmodi commented 5 years ago

Closing this in lieu of an issue: https://github.com/nitely/nim-regex/issues/41.

nitely commented 5 years ago

If other projects are enforcing this style thing and regex does not, it means regex may break others code (or at least CI pipeline) on minor versions bumps. So, I either want to enforce this, or keep it broken.

I can understand. You don't have to accept this PR as it's trivial. Feel free to commit these changes yourself. Once you create the above config.nims, you'll know what all needs to be fixed.

I meant, I don't want more PRs fixing style issues. I don't mind this particular one as it's the first one. But the only way I'm merging this is if it also contains whatever it's needed to enforce the styling.

kaushalmodi commented 5 years ago

I don't mind this particular one as it's the first one.

OK.

But the only way I'm merging this is if it also contains whatever it's needed to enforce the styling.

I would like to enforce the styling, but I cannot until https://github.com/nim-lang/Nim/issues/11756 is fixed.

A workaround would be to change the reSet enum to something else. I think it would be fine as the NodeKind type is used only internally in regex.nim. Can you suggest a different name to replace reSet. I know it's silly, but that's the only way we can enforce the styleCheck as of now.

kaushalmodi commented 5 years ago

@nitely Please take a look at this PR. It now enforces the styleCheck, though only when using Nim 0.20.0 or newer.

kaushalmodi commented 5 years ago

OK, I see the PR failing nimble test on Nim 0.20.0.. this is going to be a slow painful process while packages in stdlib get styleCheck complaint.. please bear with me.

I'll attempt to fix the stdlib packages as I get a chance.

kaushalmodi commented 5 years ago

Once https://github.com/nim-lang/Nim/pull/11762 is merged, nimble test for this project will pass on nim devel.

nitely commented 5 years ago

Once nim-lang/Nim#11762 is merged, nimble test for this project will pass on nim devel.

I think you'll have hard time enforcing styling in your projects if nim does not enforces it in its CI. A future nim release may no longer pass the style check.

Hopefully, this package is tested by nim's CI, so I'm not too worry about that.

nitely commented 5 years ago

I'll try it on devel later and marge it, thank you!

kaushalmodi commented 5 years ago

@nitely Can you add nim devel testing to your Travis as well?

nitely commented 5 years ago

No, I don't think testing against devel makes sense, that would only prove it worked with devel at some point and I'm not committing changes every day to make sure it always works. Also, I don't want contributors to fix devel issues likely unrelated to their contribution.

kaushalmodi commented 5 years ago

btw, my PR's to nim devel have been merged. Now nimble test passes for me on devel too.