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.

Sign up for GitHub

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

Jump to bottom

Add DontImplCloneOnSqlType attribute. #94

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

Add DontImplCloneOnSqlType attribute. #94

wants to merge 5 commits into from
+56 −3

Conversation

yagince
Copy link

@yagince yagince commented Dec 26, 2023

Add DontImplCloneOnSqlType attribute.
Because, if schema definitions are separated into other crate, implementing Clone for ExistingType will happen compile error.

cf. #93

@yagince
Add DontImplCloneOnSqlType attribute.
10f8e42 
Because, if schema definitions are separated into other crate, implementing Clone for ExistingType will happen compile error.
cf. adwhit#93
@yagince
Copy link
Author

yagince commented Jan 9, 2024

@adwhit @Ten0 How about this PR?

Ten0
Ten0 approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@Ten0 Ten0 left a comment

Choose a reason for hiding this comment

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

Ah I don't follow PRs here unless tagged but maybe I should ^^

.github/workflows/CI.yaml Outdated
@@ -68,6 +68,13 @@ jobs:
args: --manifest-path tests/Cargo.toml --features sqlite

- name: Install Diesel-CLI
if: ${{ matrix.rust != 'stable' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this change is necessary

Copy link
Author

Choose a reason for hiding this comment

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

If matrix.rust is not stable, rustc is an old version.
So the latest diesel-cli dependency packages cannot be compiled.
e.g. serde_spanned@v0.6.5 needs rustc 1.67 or newer

Then, I changed it as follows.
If matrix.rust is stable, use the latest dielse-cli.
If it is not stable, use the older version of diesel-cli.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I guess I'd want to just bump the MSRV then, but no strong opinion.

Copy link
Author

Choose a reason for hiding this comment

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

If I may, I would like to bump the MSRV.
However, I did this because I thought there might be some reason to keep the MSRV at 1.65.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to bump it: https://github.com/diesel-rs/diesel/blob/2.1.x/rust-toolchain is at 1.68.0
:)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your advice.
I will bump the MSRV to 1.68.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to install diesel-cli@2.1.1 with rust:1.68.0.
Then it failed to compile.

❯ docker run --rm -it rust:1.68.0 cargo install --features postgres --no-default-features diesel_cli
    Updating crates.io index
  Downloaded diesel_cli v2.1.1
  Downloaded 1 crate (53.8 KB) in 0.25s
  Installing diesel_cli v2.1.1
  Downloaded anstyle-query v1.0.2
  Downloaded autocfg v1.1.0
  Downloaded anstyle-parse v0.2.3
  Downloaded anstream v0.6.7
  Downloaded aho-corasick v1.1.2
  Downloaded unicode-normalization v0.1.22
  Downloaded unicode-bidi v0.3.14
  Downloaded diesel_migrations v2.1.0
  Downloaded serde v1.0.195
  Downloaded anstyle v1.0.4
  Downloaded unicode-ident v1.0.12
  Downloaded overload v0.1.1
  Downloaded clap_complete v4.4.6
  Downloaded tinyvec_macros v0.1.1
  Downloaded dotenvy v0.15.7
  Downloaded byteorder v1.5.0
  Downloaded toml_datetime v0.6.5
  Downloaded memchr v2.7.1
  Downloaded diffy v0.3.0
  Downloaded iana-time-zone v0.1.59
  Downloaded bitflags v2.4.1
  Downloaded pq-sys v0.4.8
  Downloaded itoa v1.0.10
  Downloaded form_urlencoded v1.2.1
  Downloaded diesel_table_macro_syntax v0.1.0
  Downloaded equivalent v1.0.1
  Downloaded migrations_internals v2.1.0
  Downloaded colorchoice v1.0.0
  Downloaded serde_spanned v0.6.5
  Downloaded utf8parse v0.2.1
  Downloaded migrations_macros v2.1.0
  Downloaded serde_regex v1.1.0
  Downloaded clap_lex v0.6.0
  Downloaded heck v0.4.1
  Downloaded strsim v0.10.0
  Downloaded nu-ansi-term v0.46.0
  Downloaded quote v1.0.35
  Downloaded percent-encoding v2.3.1
  Downloaded diesel_derives v2.1.2
  Downloaded tinyvec v1.6.0
  Downloaded num-traits v0.2.17
  Downloaded proc-macro2 v1.0.76
  Downloaded indexmap v2.1.0
  Downloaded serde_derive v1.0.195
  Downloaded clap v4.4.16
  Downloaded toml v0.7.8
  Downloaded url v2.5.0
  Downloaded toml_edit v0.19.15
  Downloaded hashbrown v0.14.3
  Downloaded clap_builder v4.4.16
  Downloaded winnow v0.5.34
  Downloaded chrono v0.4.31
  Downloaded regex v1.10.2
  Downloaded syn v2.0.48
  Downloaded idna v0.5.0
  Downloaded regex-syntax v0.8.2
  Downloaded regex-automata v0.4.3
  Downloaded diesel v2.1.4
  Downloaded 58 crates (4.4 MB) in 0.45s
error: failed to compile `diesel_cli v2.1.1`, intermediate artifacts can be found at `/tmp/cargo-install7WzKFd`

Caused by:
  package `clap v4.4.16` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.68.0
  Try re-running cargo install with `--locked`

Could I bump to 1.70 ?
I was able to compile it with 1.70.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion here, I'm going to let @adwhit decide on this :)

Copy link
Author

Choose a reason for hiding this comment

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

@adwhit What do you think about this topic?

src/lib.rs Outdated
@@ -115,6 +126,10 @@ fn val_from_attrs(attrs: &[Attribute], attrname: &str) -> Option<String> {
None
}

fn contains_in_attrs(attrs: &[Attribute], attrname: &str) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just has_attr?

Copy link
Author

Choose a reason for hiding this comment

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

I imitated val_from_attrs, but that is a better idea.

diesel-derive-enum/src/lib.rs

Lines 96 to 116 in 36c44a2

fn val_from_attrs(attrs: &[Attribute], attrname: &str) -> Option<String> {
for attr in attrs {
if attr.path().is_ident(attrname) {
match &attr.meta {
Meta::NameValue(MetaNameValue {
value:
Expr::Lit(ExprLit {
lit: Lit::Str(lit_str),
..
}),
..
}) => return Some(lit_str.value()),
_ => panic!(
"Attribute '{}' must have form: {} = \"value\"",
attrname, attrname
),
}
}
}
None
}

I will change it to has_attr.

@yagince
Copy link
Author

yagince commented Jan 10, 2024

@Ten0 Thank you for taking the time to review!

@yagince
Copy link
Author

yagince commented Jan 30, 2024

@adwhit
I bumped the MSRV to 1.70
Because I needed 1.70 or newer to install diesel_cli@2.1.1.

@rac-sri
Copy link

rac-sri commented Jul 29, 2024

Can we bump up this PR review please. Running into the same issue.

@brianmay brianmay mentioned this pull request Jan 6, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ten0 Ten0 Ten0 approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yagince @rac-sri @Ten0