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

trait Serialize uses reference to self instead of move. #37

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

Grinkers
Copy link
Collaborator

@Grinkers Grinkers commented Dec 23, 2023

Blocked by #35 (commits included here). Will need to be cleaned up.

Needs to be paired with naomijub/edn-rs#137

@Grinkers
Copy link
Collaborator Author

btw I can't push branches on this repo. Maybe we can get similar rules to edn-rs?
@naomijub @evaporei

@evaporei
Copy link
Owner

btw I can't push branches on this repo. Maybe we can get similar rules to edn-rs? @naomijub @evaporei

Fixed :)

@Grinkers Grinkers requested a review from evaporei December 24, 2023 21:59
@Grinkers Grinkers force-pushed the no_move branch 2 times, most recently from df8637a to e4ea7c7 Compare December 24, 2023 22:14
@Grinkers
Copy link
Collaborator Author

The commits in edn-rs are now part of
naomijub/edn-rs#140 because of naomijub/edn-rs#139

I wrote my notes for the upstream issue at Grinkers/edn-rs#4

https://github.com/edn-rs/edn-derive/pull/37/files#diff-5522b0b8feca57114b7b725a538b565a55e1f7acab45d45b58402a8901879aef
You can see an example of the issue here. It's an unreasonable expectation to rely on users to implement clone for all types.

@evaporei I'm willing to break things apart into smaller PR's on edn-rs if you are able to review them. Right now everything is piling up and can't be isolated without conflicts. I'm not going to die on the hill of serialize() vs edn_rs::from_str when we can have both, but it's becoming more work maintaining all history and conflicts than actually working on new things.

@Grinkers
Copy link
Collaborator Author

naomijub/edn-rs#144
has been merged. This is a cleaned up version, rebased from #41
so actually we can just merge this in place of both #40 and #41

@Grinkers Grinkers marked this pull request as ready for review February 12, 2024 21:47
@Grinkers Grinkers force-pushed the no_move branch 4 times, most recently from e580f12 to 656c692 Compare February 14, 2024 19:01
@Grinkers Grinkers marked this pull request as draft February 14, 2024 19:05
@Grinkers Grinkers marked this pull request as ready for review February 14, 2024 20:54
@Grinkers
Copy link
Collaborator Author

Fixes to pair with naomijub/edn-rs#144

Sorry for all the force pushes. I confused myself with us temporarily using git references along with a local issue with trybuild.

@Grinkers
Copy link
Collaborator Author

I also tacked on a commit for an empty .rustfmt.toml file and outdated deps. Tell me if there's any issues and I can break it apart.

@evaporei evaporei merged commit 93d2122 into evaporei:main Feb 15, 2024
4 checks passed
@Grinkers Grinkers deleted the no_move branch February 15, 2024 19:42
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