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

Added schema parameters to functions in join and select #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

skytz
Copy link

@skytz skytz commented Jun 13, 2023

  • also bumped the version number to ilustrate the changes

@kurtbuilds
Copy link
Owner

kurtbuilds commented Jun 15, 2023

Hey, sorry I thought I already left a comment on this.

Most people are going to infrequently, if at all, use schemas.

Because of that, I don't think it makes sense to have most people pass a None parameter, which would cause confusion.

Few possible workarounds:

  1. The functions take an S: Into<Table> or something, which &str would implement. Strings could get parsed to split schema if there's a ..
  2. Adding secondary convenience methods like from_table_with_schema

Something else?

@skytz
Copy link
Author

skytz commented Jun 24, 2023

I'm not a big fan of magic strings, so the first approach was too close to that for me.
The only good solution for me would be the 2nd one, with "with_schema" at the end. It's explicit.
I've also reverted all the formatting issues.

Copy link
Owner

@kurtbuilds kurtbuilds left a comment

Choose a reason for hiding this comment

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

Two small comments. Once that's done, happy to merge & cut release.

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "sqlmo"
version = "0.11.4"
version = "0.12.4"
Copy link
Owner

Choose a reason for hiding this comment

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

For 0.x, the 2nd number is a major version.

Since this is just adding names, I think this can just be a minor bump. 0.11.5. (if it is breaking, the next version would be 0.12.0.)

That said, I'm happy to bump version for a cut after merge.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, you're right, it doesn't break compatibility, it did before the added functions

@@ -69,6 +69,11 @@ impl Select {
self
}

pub fn table_column_with_schema(mut self, schema: Option<&str>, table: &str, column: &str) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

One adjustment I'd make: since the function explicitly is called _with_schema, I think it makes sense to take a &str instead of Option<&str>.

Likewise for the other method names.

Copy link
Author

Choose a reason for hiding this comment

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

the problem with taking a &str is that in external calls (specifically in ormlite)..you need to have an if or a match to check for schema when creating the objects, in all cases. It's a bit...ugly.
I'm ok with either one, you pick.

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.

2 participants