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

Improve pluralizations for Documentation and SqlOperation NodeTypes #5356

Merged
merged 4 commits into from
Jun 30, 2022

Conversation

pdebelak
Copy link
Contributor

@pdebelak pdebelak commented Jun 9, 2022

resolves #5352

Description

Previously these were docss and sqlss which leaves something to be desired.

Checklist

Previously these were `docss` and `sqlss` which leaves something to be
desired.
@pdebelak pdebelak requested a review from a team as a code owner June 9, 2022 16:41
@pdebelak pdebelak requested review from VersusFacit and iknox-fa June 9, 2022 16:41
@cla-bot
Copy link

cla-bot bot commented Jun 9, 2022

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @pdebelak

@cla-bot
Copy link

cla-bot bot commented Jun 9, 2022

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @pdebelak

@pdebelak
Copy link
Contributor Author

After checking with my employer, I've now been able to sign the CLA.

@cla-bot cla-bot bot added the cla:yes label Jun 10, 2022
@jtcohen6 jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Jun 14, 2022
@leahwicz leahwicz closed this Jun 30, 2022
@leahwicz leahwicz reopened this Jun 30, 2022
@leahwicz
Copy link
Contributor

Closing/reopening to trigger CI

Comment on lines 60 to 64
if self is self.Documentation:
return "docs blocks"
if self is self.SqlOperation:
return "sql operations"
return f"{self}s"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self is self.Documentation:
return "docs blocks"
if self is self.SqlOperation:
return "sql operations"
return f"{self}s"
return f"{self}s"

Instead, let's just rename the node strings more appropriately:
lines 15 to SqlOperation = "sql operation"
and line 16 to Documentation = "docs block"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated.

Copy link
Contributor

@leahwicz leahwicz left a comment

Choose a reason for hiding this comment

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

Thanks for the contributtion @pdebelak!

@leahwicz leahwicz merged commit 60e491b into dbt-labs:main Jun 30, 2022
@pdebelak pdebelak deleted the improve-node-type-pluralizations branch July 1, 2022 01:52
gshank pushed a commit that referenced this pull request Jul 6, 2022
…5356)

* Improve pluralizations for Documentation and SqlOperation NodeTypes

Previously these were `docss` and `sqlss` which leaves something to be
desired.

* Add changie changelog entry for pluralization change

* Slighly simplify node type pluralization tests

* Update node type names for sql and docs so they match pluralizations
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
…bt-labs#5356)

* Improve pluralizations for Documentation and SqlOperation NodeTypes

Previously these were `docss` and `sqlss` which leaves something to be
desired.

* Add changie changelog entry for pluralization change

* Slighly simplify node type pluralization tests

* Update node type names for sql and docs so they match pluralizations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ok_to_test ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-737] [Bug] Wonky pluralization when duplicate docs are found
3 participants