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 translations for string.Substring with one arg #24746

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

stevendarby
Copy link
Contributor

Fixes #20173

@dnfadmin
Copy link

dnfadmin commented Apr 23, 2021

CLA assistant check
All CLA requirements met.

@stevendarby
Copy link
Contributor Author

stevendarby commented Apr 27, 2021

Sorry for delay on this, as a first timer I’m just waiting on a confirmation around the two options on the CLA, should end up fine either way. CLA signed

@stevendarby
Copy link
Contributor Author

@ajcvickers Let me know if there’s anything else I need to do to unblock this.

Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Since these are in projection so for cosmos/sqlite just pass even though not translating. We should add tests where they are used in where. @maumar will provide guidance on that.

@stevendarby
Copy link
Contributor Author

Thanks, I think I had a misconception that abstract class tests weren’t discovered.

I’ll take a closer look and can make a start improving the testing as suggested.

@stevendarby
Copy link
Contributor Author

stevendarby commented Apr 29, 2021

  • Decided to just go ahead and implement for Cosmos and Sqlite too.
  • Sqlite doesn't need 3rd argument so don't have to do a length function
  • Tests now use the substring in the where.
  • the substring with IndexOf test started to break, but wasn't sure why - seemed to be problem in the test framework? Appreciate any help there. For now I removed as only had that one after basing tests on the 'two arg' substring, perhaps it isn't really needed in the substring tests.
  • Happy to take any further guidance, hopefully this is a step in the right direction though.

@stevendarby stevendarby changed the title Add SqlServer translation for string.Substring with one arg Add translations for string.Substring with one arg Apr 29, 2021
@smitpatel smitpatel merged commit 67a5d16 into dotnet:main Apr 30, 2021
@smitpatel
Copy link
Member

@stevendarby - thank you for contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Substring with single parameter throws InvalidOperationException when adding Where clause
4 participants