Skip to content

Translate {PadLeft,PadRight} to {lpad,rpad} - #673

Merged
austindrenski merged 2 commits into
npgsql:devfrom
austindrenski:translate-lpad-rpad
Oct 21, 2018
Merged

Translate {PadLeft,PadRight} to {lpad,rpad}#673
austindrenski merged 2 commits into
npgsql:devfrom
austindrenski:translate-lpad-rpad

Conversation

@austindrenski

Copy link
Copy Markdown
Contributor

Closes: #669

@rojiroji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Look good, thanks!

One note though, before merging... Instead of adding a whole new test class + fixture, why not put these tests in SimpleQueryNpgsqlTest? This is where the rest of the string methods are tested (e.g. String_StartsWith_Literal). I'm a bit concerned we're adding fixtures and test classes left and right, and it's going to be a pain to maintain.

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

One note though, before merging... Instead of adding a whole new test class + fixture, why not put these tests in SimpleQueryNpgsqlTest?

I wasn't sure what the norms were for adding provider-specific tests to the core tests (in most cases, it looks like we just override, rather than add to).

But if it's okay to add new tests, happy to do so! I'll update now.

@austindrenski
austindrenski merged commit 1b72c4d into npgsql:devOct 21, 2018
@roji

roji commented Oct 21, 2018

Copy link
Copy Markdown
Member

Looks great, thanks for making the change!

@austindrenski
austindrenski deleted the translate-lpad-rpad branch October 23, 2018 14:49
Sign up for freeto 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

@austindrenski@roji