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

[dev] Make mockgun string comparison case insensitive (RDEV-15305) #20

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

rlessardrodeofx
Copy link

@rlessardrodeofx rlessardrodeofx commented Oct 29, 2019

Purpose of the PR

This PR ensure that mockgun comparison of text fields are case insensitive.
This is the case for a real Shotgun instance.

After this is approved I will do an official PR upstream.

Overview of the changes

  • Do lowercase comparison for text fields
  • Add unit-tests

Type of feedback wanted

Any kind of feedback.
Is there some other situations where case sensitivity matter that I did not test?

Where should the reviewer start looking at?

Diff should be enough

Potential risks of this change

Some tests that where using mockgun could start failing (which in my case is what I want).

Relationship with other PRs

None

Copy link

@Zaltu Zaltu left a comment

Choose a reason for hiding this comment

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

I don't see a reason to not do this for us here. In terms of eventually pushing this upstream, I believe based on the recent updates to Shotgun itself that the longer-term goal for them is to let the user decide whether a field should be case-sensitive or not (which as you know is already the case for filed tagged as unique per project).
This would probably be a good thing to open a ticket about and ask them their official plans for. I can do that if that sounds right.

Copy link

@swaugh-rodeo swaugh-rodeo left a comment

Choose a reason for hiding this comment

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

Woop forgot which account I was on

@rlessardrodeofx
Copy link
Author

This would probably be a good thing to open a ticket about and ask them their official plans for. I can do that if that sounds right.

That would be great! If such an option appear I expect it to be visible in the schema and in that case the code should be straightforward to adapt.

@JeanChristopheMorinRodeoFX

@rlessardrodeofx Do we have a PR opened on the upstream repo for this? What do the Shotgun guys think?

@rlessardrodeofx
Copy link
Author

I've opened a PR upstream here:
shotgunsoftware#217

@JeanChristopheMorinRodeoFX
Copy link

JeanChristopheMorinRodeoFX commented Nov 4, 2019

Thanks, can you update the ticket we have with Shotgun with the link to the PR so they have someone on their end that take some time to look at it please? See https://support.shotgunsoftware.com/hc/en-us/requests/106863

@rlessardrodeofx
Copy link
Author

Done!

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