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

Add exported function to remove rules #316

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

ldebruijn
Copy link
Contributor

Hey there!

I'm using gqlparser in another project. There, I add a number of custom rules on top of the existing ones.
I'm having an issue with writing tests though. Since the rules global variable is not exported, and there exists no RemoveRule functionality, my tests start to interfere with each other as this global state is never reset between test cases.

For that reason, I'd like to add this MR, so I can clear this state between runs.

Things I've considered:

  • Exporting the rules global variable. Decided against that as that would be a much more impactful change
  • Adding a 'ClearRules' or 'ResetRules' function, that would reset to the initial state provided by this library. Decided against that as that would require changing the current registrating of rules provided by this library through init() functions, to something else so that they can be re-initialized.
  • Exporting a RemoveRule that does a string-based match on the rule name, and remove it (actually filter it out on copy) from the list. Then re-assign the still unexported field to this new, filtered, list.

Adding meaningful tests for this was hard as the tests for the validator are not in the same package (validator vs validator_test). I added tests to assure there are no errors when running the RemoveRule function both when there is a match on a rule, and when there is not.

If a different approach is preferred I'm happy to alter the MR. I thought I'd start small and get your thoughts on the right strategy before trying to rewrite too much.

I have:

  • Added tests covering the bug / feature
  • Updated any relevant documentation

@coveralls
Copy link

coveralls commented Sep 13, 2024

Coverage Status

coverage: 87.544% (+0.02%) from 87.527%
when pulling 2f8f28f on ldebruijn:master
into f0827b6 on vektah:master.

@StevenACoffman
Copy link
Collaborator

Hi! Thanks for this work. I think adding a "RemoveRule" is great, but I went ahead and merged #320 because I do believe there is a need to replace the rules completely occasionally (non-parallel tests, for instance).

However, that caused merge conflicts with your PR here.

@ldebruijn
Copy link
Contributor Author

Hi! Thanks for this work. I think adding a "RemoveRule" is great, but I went ahead and merged #320 because I do believe there is a need to replace the rules completely occasionally (non-parallel tests, for instance).

However, that caused merge conflicts with your PR here.

Hi Steve, just dropping by to say Ive seen your comment and will fix the conflicts in the PR. I currently don’t have access to a laptop for a few weeks, but will get to it once I do.

@ldebruijn
Copy link
Contributor Author

@StevenACoffman all good to be merged now!

@StevenACoffman StevenACoffman merged commit c888731 into vektah:master Oct 14, 2024
5 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks very much for your patience and contribution! I really like where we ended up with the functionality.

@fredzqm
Copy link
Contributor

fredzqm commented Oct 15, 2024

Thank you! I was looking to this feature as well!

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.

4 participants