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

Evaluate @ignore tags case insensitively #1750

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Evaluate @ignore tags case insensitively #1750

merged 3 commits into from
Aug 31, 2021

Conversation

treyturner
Copy link
Contributor

Description

This simple PR allows for case-insensitive evaluation of the built-in @ignore tag. We use camel-cased annotations everywhere else so it would be nice if we didn't have to remember this exception.

If you're interested in merging this but feel tests would be appropriate then I am happy to add those - it just seemed like probable overkill.

Thanks for your consideration and for your work on Karate.

  • Type of change :
    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation

@ptrthomas
Copy link
Member

@treyturner hmm isn't this easier for you to do via a find and replace in your code base. the reason I'm hesitating is that this implies that we should do a case-insensitive comparison for all tags (and values !?)

@treyturner
Copy link
Contributor Author

We can certainly do that; I just thought it might be nice to implement for the recently added built-in @ignore tag since my testers are used to using Spock's @Ignore annotation.

There's no pressure to merge this, it's just something I noticed as I attempted to remove our manual ~@Ignore tag and saw that wasn't working as I expected. I didn't mean to imply that all keys or values should be case insensitive.

@ptrthomas ptrthomas merged commit a91b4af into karatelabs:develop Aug 31, 2021
@ptrthomas
Copy link
Member

@treyturner merged but was really not keen. may revert in the future if any concerns arise in the wild

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