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

feat(api): relabel columns to ALL_CAPS #6532

Closed
wants to merge 0 commits into from
Closed

feat(api): relabel columns to ALL_CAPS #6532

wants to merge 0 commits into from

Conversation

IndexSeek
Copy link
Member

Using "snake_case" has been a useful short-hand as was implemented in #5336. In addition to "snake_case" I wanted to add support for "screaming_snake_case". Some systems, such as Snowflake, Oracle SQL, and PL/SQL follow this sort of convention.

I think this would be a worthwhile addition that might help when working with different backends or databases that treat double-quoted identifiers as case-insensitive.

https://en.wikipedia.org/wiki/Snake_case

https://docs.snowflake.com/en/sql-reference/identifiers-syntax#migrating-from-databases-that-treat-double-quoted-identifiers-as-case-insensitive

@lostmygithubaccount
Copy link
Member

should it be "SCREAMING_SNAKE_CASE"?

@IndexSeek
Copy link
Member Author

should it be "SCREAMING_SNAKE_CASE"?

I thought about this, as that makes sense, as it's usually how it's stylized. I'm happy to make that tweak if that's how it would be preferred to be referenced.

It looks like it could also be referred to as the following:

  • ALL_CAPS
  • MACRO_CASE
  • CONSTANT_CASE

But in each case, the styling is all caps, as you mention.

@cpcloud
Copy link
Member

cpcloud commented Jun 27, 2023

@IndexSeek Nice, thanks for the PR.

Can we go with ALL_CAPS? SCREAMING_SNAKE_CASE is kind of a handful to type and ideally we can keep these shorthands ... short 😂.

@cpcloud cpcloud added feature Features or general enhancements ux User experience related issues labels Jun 27, 2023
@IndexSeek
Copy link
Member Author

@IndexSeek Nice, thanks for the PR.

Can we go with ALL_CAPS? SCREAMING_SNAKE_CASE is kind of a handful to type and ideally we can keep these shorthands ... short 😂.

Definitely! I have made that change now and agree, short and simple. I thought the original "snake_case" short-hand was really handy; I'm hoping this version of it will also be convenient, especially for going between backends.

@IndexSeek IndexSeek changed the title feat(api): relabel columns to screaming snake case feat(api): relabel columns to ALL_CAPS Jun 27, 2023
@cpcloud
Copy link
Member

cpcloud commented Jun 27, 2023

@IndexSeek You may want to disable GitHub Actions on your fork. I believe that will cause a bit of CI congestion for you since you'll be running every job twice: once against the main ibis repo and once against your fork.

@cpcloud cpcloud added this to the 6.0 milestone Jun 27, 2023
@cpcloud
Copy link
Member

cpcloud commented Jun 27, 2023

@IndexSeek I think there may be a whitespace change picked up by the markdown autoformatter. I'll fix it up.

@cpcloud
Copy link
Member

cpcloud commented Jun 27, 2023

Hm, looks like I can't push to the branch because it's master (I can only push to non-master branches).

@IndexSeek
Copy link
Member Author

@IndexSeek You may want to disable GitHub Actions on your fork. I believe that will cause a bit of CI congestion for you since you'll be running every job twice: once against the main ibis repo and once against your fork.

This is a good call, disabled them now, that'll help keep my email inbox a little cleaner as well.

@IndexSeek I think there may be a whitespace change picked up by the markdown autoformatter. I'll fix it up.
Hm, looks like I can't push to the branch because it's master (I can only push to non-master branches).

I've made the whitespace fix now, thank you for looking at that and attempting to fix it. I should have just made a feature branch on my fork. Hopefully it is good now, checks are still queued as I'm commenting.

@cpcloud cpcloud requested a review from jcrist June 27, 2023 16:36
Copy link
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

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

One small type annotation nit, otherwise this looks good to me!

Could you please squash your commits together into a single commit (with a message something like "feat(api): add ALL_CAPS option to Table.relabel"). We generate our changelog from commit messages, so we're a bit picky here about this.

Comment on lines 1659 to 1660
| Literal["snake_case"]
| Literal["ALL_CAPS"],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Literal["snake_case"]
| Literal["ALL_CAPS"],
| Literal["snake_case", "ALL_CAPS"],

@IndexSeek
Copy link
Member Author

I've opened #6543 to squash those commits and try this again. I truly appreciate all of your patience and support. I understand wanting to keep things simple with the CHANGELOG.

As I'm commenting on this, I realize I must combine the Literal typing as was suggested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants