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 missing Clone trait #24

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Conversation

jayvdb
Copy link
Collaborator

@jayvdb jayvdb commented Jan 16, 2023

No description provided.

@Electron100
Copy link
Owner

Thanks for the PR! Clone on Many makes sense, but Default on ForeignKey doesn't seem quite right. An empty default isn't really meaningful. Today ForeignKey enforces the internal invariant that either its val or valpk field is set. Initializing both of them to empty/None will result in an invalid state on which several methods will panic.
Can you help me understand what you're hoping to achieve? We may be able to find a tweak that meets the same goal without putting the structure into a panic-prone state.

@jayvdb
Copy link
Collaborator Author

jayvdb commented Jan 16, 2023

I would like some implementation of Default on ForeignKey, as I need Default on the structs which use a ForeignKey for one of the members, so all members type need Default. This requirement is not under my control - the structs are used by other tools which require Default. Using #[derive(..)] did the trick for me, but I can appreciate that the simplistic implementation from #[derive(..)] may cause panics which I haven't found because I have limited use for it unrelated to butane.

@Electron100
Copy link
Owner

Thanks for the explanation. I understand the constraint from another library -- the problem though is that a ForeignKey initialized without any information is semantically invalid: it's essentially uninitialized and you can't do anything with it. I think you have a couple options within your code.

  1. You could use an Option<ForeignKey<T>>. Butane should handle that fine (if you see issues, I'll treat it as a bug) and semantically expresses the reality that you may or may not have a valid ForeignKey. Of course when used in a #[model], it also makes the column in your database nullable -- that may or may not be what you want here.
  2. Your impl of Default could call ForeignKey::from_pk and pass 0, or empty string, or zero'd UUID, or whatever's most appropriate for the primary key you're using. Of course you still have an invalid ForeignKey that you can't do anything with, but now you've put that concept only into your application code where you can ensure you account for it judiciously, rather than pushing it into Butane where it could easily become a footgun.

@jayvdb jayvdb force-pushed the add-useful-derives branch from 09a0736 to c3fcc15 Compare January 16, 2023 23:54
@jayvdb jayvdb changed the title Add missing Debug and Default traits Add missing Clone trait Jan 16, 2023
@jayvdb
Copy link
Collaborator Author

jayvdb commented Jan 16, 2023

ok, I've removed the Default. Thanks for your thoughts about that. I am going to need mandatory fkeys, so I'll chew on that problem for a bit.

@Electron100 Electron100 merged commit 093036d into Electron100:master Jan 17, 2023
@jayvdb jayvdb deleted the add-useful-derives branch June 14, 2023 06:38
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