Question about upstreaming changes from fork

[hasezoey]

Hello, because this project was inactive for some time, i had made a fork and added some features to it, i would like to know if i should upstream the changes and when yes which?

list of changes can be found in the forks CHANGELOG

PS: i know this should likely not be a issue, but github discussions is not enabled for this project

[Wulf]

hey @hasezoey, thanks for reaching out. It's really great to see some of those changes which were on my mind (especially common.rs). I've added you as a collaborator to this repository. Apart from that, could we discuss the following changes?

• add option only-necessary-derives to only generate the necessary diesel derives for a struct
  • How does this determine which derives are necessary? Is it a hard-coded list?
• "add option once-connection to only output the connection type once in common.rs instead of in all files"
  • Awesome change, let's merge it in. I was wondering if there was a reason you didn't make it the default?
• also, can you explain the use-case for these changes?
  • add option lessen-conflicts to lessen conflict with diesel types (like having to import Connection)
  • add option file-mode to set a different mode than direct overwrite

Feel free to pull any of the rest in and let me know if you need help with anything

[hasezoey]

I was wondering if there was a reason you didn't make it the default?

in the fork, with every option added i tried to keep the default as the upstream version would generate, but have the option to change it

How does this determine which derives are necessary? Is it a hard-coded list?

yes you could say it is a hardcoded-list, "necessary" as in only have the diesel derives which are actually necessary for the particular struct, like a Create* struct would need Insertable and AsChangeSet(maybe, i dont know), a Update would need AsChangeSet and the normal struct would need Selectable, Queryable etc (but not Insertable)

add option lessen-conflicts to lessen conflict with diesel types (like having to import Connection)

as with the previous question on why i didnt make it the default, lessen-conflicts was meant to lessen conflicts with diesel types, like renaming dsync Connection to ConnectionType so it dose not conflict with a import of diesel::Connection
also lessen-conflicts changes to that it is no longer common::* import, which could arguably be always that way

add option file-mode to set a different mode than direct overwrite

i made a file-mode so that the user can decide how to handle the files (3 options, overwrite, dsync_new, none), but personally i didnt actually use it and though it would be a nice-to-have, but could certainly be left out (dsync_new like .dpkg-new or .pacnew on linux)

[hasezoey]

for the first round of changes i have made the bare things required to get a proper setup going and base changes that other things will require / would be good to have:

• Maintenance changes #57
• Re-enable clippy & fmt (also add helper scripts) #58
• Lessen amount of panics #59

[hasezoey]

btw i noticed that only squash and merge is enabled for this repository, is this really a good way to merge things like the mentioned changes as just one commit?

---

as for the next changes, i would like to know before doing them if they are actually wanted:

• switch from structopt to clap
  • this would allow doing subcommands like generating auto-completions
  • personally i think the help and definition of values / structs is better
• split binary and library
  • this would allow the binary to always enable things like feature backtrace
  • would allow the library to have less dependencies that it wont need (like structopt/clap, anyhow)

[Wulf]

Thanks for the replies! I appreciate that you thought of backwards-compatibility.

Let's make lessen-conflict, once-connection and only-necessary-derives the default. As for file-mode, we can keep it to the side for now and reconsider it if it's really necessary. Let's keep the feature surface small -- it'll make it easier for us to maintain this moving forward.

"squash and merge" is great for small projects like this, but if you'd like, I can re-enable merge commits. Let me know :)

I'm okay with the other two changes as well.

For 'split binary and library', although it'll increase complexity, I'm hoping compile time will decrease for those using dsync as a library.

[hasezoey]

"squash and merge" is great for small projects like this, but if you'd like, I can re-enable merge commits. Let me know :)

i think rebase and merge would be good for this, it will keep the commits intact, but lessens the amount of "mess" in the git history (git log). intact commit should make git blame and git bisect (if necessary) easier to find the cause of instead of massive commits

[Wulf]

Done :)

[hasezoey]

i think rebase and merge would be good for this

actually now that i look at it after having merged #57, it seems like option rebase and merge will completely loose verification if the commits had it

so i guess the only other options are to manually merge it locally, squash merge or merge commit merge (or asking the PR authors to rebase manually)

github issue

[Wulf]

Let’s skip the --no-impls changes since we merged in the --no-crud PR

[hasezoey]

as for once-connection, once-common-structs and single-model-file should they be the default and unconfigurable, or should it be configurable?

TL;DR:

• once-common-structs: put common structs (like PaginationResult) in a common.rs file
• once-connection: put type ConnectionType in a common.rs file
• single-model-file: output only 1 file instead of 2 files and a directory (instead of table_name/mod.rs & table_name/generated.rs, generate a table_name.rs)

[Wulf]

Thanks for the summaries -- I think all of them except single-model-file should be default and are great changes.

The original reason for having a mod.rs and a generated.rs file was to easily support adding your own implementation to the generated output without worrying about creating a separate directory structure or files.

Someone using --no-impls or --no-crud probably doesn't need this structure so single-model-file is still useful for this usecase but because crud is the default, then so should this double-model-file structure be default.

If you think we should move to no-impls by default, then we should also move to single-model-file by default.

[hasezoey]

i basically have now all required things from the fork either merged or open as a PR, though there are some additional things from the fork:

• feature --create-str, i intend to also upstream this, but as a enum with str, String, Cow, default being String(like currently)
• feature --only-necessary-derives to only include the necessary derives on a specific struct (also includes some refactors so that all derives are defined in one place)
• feature read-only-prefix, specify a prefix like view_ which will automatically make this table "read-only" (no update, create structs or fn impls), i also intend to add a read-only-suffix, just to be consistent
• refactor some things so new-lines are consistent in output files and so that formats will look better

[hasezoey]

actually now that i look at some of the changes in the fork again, there is one thing i missed: doc-comments.

Example file

is this something we should do and when yes, can the doc-comments be improved?

[Wulf]

It's a good question. I like this idea and I think we can make it more beneficial by adding examples of how to use the structs and fns.

[hasezoey]

i am sure i now have all the changes from the fork upstreamed (excluded the extra file modes and separating library & binary), so i will now close this issue (and after a version gets released put up a note on the fork)