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 SCREAMING_SNAKE_CASE rename_all option #478

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

FuegoFro
Copy link
Contributor

@FuegoFro FuegoFro commented Jul 2, 2020

This mirrors the addition of uppercase in #304, but for all uppercase snake case.

sqlx-macros/src/derives/attributes.rs Outdated Show resolved Hide resolved
sqlx-macros/src/derives/mod.rs Show resolved Hide resolved
#[derive(PartialEq, Debug, sqlx::Type)]
#[sqlx(rename = "color_screaming_snake")]
#[sqlx(rename_all = "screaming_snake_case")]
enum ColorScreamingSnake {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #304 tests are added for multiple databases, but it seemed like that's no longer necessary to do explicitly as of e5b6047 since that removed the other tests. Let me know if that's an incorrect assumption, and I can add tests for other databases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have any standing policy right now dictating that everything must be tested on all databases. I think in general we want to keep the bar for landing contributions as low as possible while still having a minimal level of assurance that a given feature works. Not everyone is willing or able to write and run tests for all DB flavors.

For something like this that is pretty database-agnostic, I think is okay being tested on just one database. For PRs that may potentially have DB-specific issues like adding new types, we definitely want to cover all our (data)bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks for the clarification!

@phated
Copy link
Contributor

phated commented Jul 7, 2020

@FuegoFro awesome! Excited to see this! After adding uppercase, I realized I needed screaming snake for some of my enums but I've just been manually adding names.

@@ -58,6 +58,14 @@ enum ColorUpper {
Blue,
}

#[derive(PartialEq, Debug, sqlx::Type)]
#[sqlx(rename = "color_screaming_snake")]
#[sqlx(rename_all = "screaming_snake_case")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the case for this and line 54 need to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yup, my bad for missing this!

@FuegoFro
Copy link
Contributor Author

@abonander I'm fairly new to Rust dev and the errors in CI here are a bit opaque to me. They seem to be unrelated to the changes I've made, and when I try to reproduce them locally (eg by running python3 ./tests/x.py -t sqlite_tokio and python3 ./tests/x.py -t mariadb_10_4_tokio) things seem to either pass without issue, or just get a totally different set of errors than in CI. I also just rebased on master yesterday and it still seemed to fail.

Do you have any tips for how I can track down and fix these CI failures?

@mehcode
Copy link
Member

mehcode commented Jul 24, 2020

That failure looks unrelated to your PR. Thanks for the contribution. 💯

@mehcode mehcode merged commit f0f93c4 into launchbadge:master Jul 24, 2020
@FuegoFro
Copy link
Contributor Author

Thank you for the merge! 😀

@FuegoFro FuegoFro deleted the add_screaming_snake_case branch July 27, 2020 18:36
@FuegoFro FuegoFro restored the add_screaming_snake_case branch July 27, 2020 18:36
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