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

Fix some things that were broken #112

Merged
merged 5 commits into from
Nov 19, 2023
Merged

Fix some things that were broken #112

merged 5 commits into from
Nov 19, 2023

Conversation

hasezoey
Copy link
Collaborator

@hasezoey hasezoey commented Nov 2, 2023

This PR is a small collection of fixes from #104, but refactored to current state dsync and separate from the other changes and not being based on #103, in more detail:

  • always have type ConnectionType be public (was broken for once-connection-type)
  • change serde derives to use serde:: instead of requiring it to be imported (was broken for once-common-structs)

these things would likely be catched earlier if the tests would actually be compile tested

@hasezoey
Copy link
Collaborator Author

hasezoey commented Nov 2, 2023

as this pr is a collection of fixes, i added some things i noticed while doing #114, namely:

  • i noticed that diesel:PgConnection does not actually exist (at least not anymore)
  • that tables which names will be converted will have mismatching imports (directory / module is name tableA while the import was table_a)
  • added a TODO because i also noticed that the delete pass does not check single-model-file files

Copy link
Owner

@Wulf Wulf left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's merge this in instead of #104

@Wulf
Copy link
Owner

Wulf commented Nov 18, 2023

I'm going to add a few more commits here which should take care of some of the chnages which weren't included from #104

@Wulf
Copy link
Owner

Wulf commented Nov 18, 2023

ah, I can't seem to do this. Do you mind adding the following changes?

  1. Remove 'tsync' from Cargo.toml
image
  1. In src/lib.rs, add the following comment above pub connection_type: String,:
    /// For example:
    /// - `diesel::pg::PgConnection` (default)
    /// - `diesel::sqlite::SqliteConnection`
    /// - `diesel::mysql::MysqlConnection`
    /// - or, your custom diesel connection type (struct which implements `diesel::connection::Connection`)
image
  1. In src/lib.rs, add a default for GenerationConfig before impl GenerationConfig<'_> {
impl<'a> Default for GenerationConfig<'a> {
    fn default() -> Self {
        Self {
            table_options: Default::default(),
            default_table_options: Default::default(),
            connection_type: "diesel::pg::PgConnection".into(),
            diesel_backend: "diesel::pg::Pg".into(),
            schema_path: "crate::schema::".into(),
            model_path: "crate::models::".into(),
            once_common_structs: true,
            once_connection_type: true,
        }
    }
}
image

Aaandddd, that should be all! 🙌

If you're not able to do this, no worries, I can add it in after we merge this in. Just let me know!

which was broken for "once-connection-type"

re Wulf@cc0ecf2
diesel does not have "diesel::PgConnection", only in "::prelude" or "::pg"
fixes mismatch between a tables imports and the directory structure
@hasezoey
Copy link
Collaborator Author

hasezoey commented Nov 19, 2023

rebased on latest main, and added the 2. for examples, removing the (default) and adding r2d2, also adding that comment to the binary help.
i also changed the previous diesel::prelude::PgConnection to diesel::pg::PgConnection (as requested in #114 (comment))

as for 1. remove tsync feature default and 3. GenerationConfig default derive, this should likely be done in a separate PR
(the tsync attribute is not added by default and would require a argument (--tsync) in addition to having the feature enabled; generation config default derive would likely need some refactoring to make it work with good defaults (like connection_type)

@hasezoey hasezoey merged commit 830655e into Wulf:main Nov 19, 2023
6 checks passed
@hasezoey hasezoey deleted the fixes branch November 19, 2023 12:58
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