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

Refactor the renaming code #1429

Closed
wants to merge 2 commits into from

Conversation

Phlosioneer
Copy link

It's now concentrated in the Name struct. Some clones were avoided,
too.

Partly in preparation for #1427

It's now concentrated in the Name struct. Some clones were avoided,
too.

Partly in preparation for serde-rs#1427
Forgot to check backwards compatibility
@Phlosioneer
Copy link
Author

Oops, forgot to check back for test failures.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! I am closing the PR because it doesn't seem like I will have bandwidth to own the additional surface area of a public serde_derive_internals. It would work better to do this development outside of the Serde repo under a different crate name. Then when the API has settled and there are a variety of use cases implemented against it, we can look at whether it makes sense to bring any of it back into the copy of serde_derive_internals that is used by serde_derive itself.

@dtolnay dtolnay closed this Dec 1, 2018
@Phlosioneer
Copy link
Author

Phlosioneer commented Dec 2, 2018

Well then... how do you propose to make any progress on #1427? If the API can't be changed, then trying to make room for plugins is pointless; they would have to re-do much of the work in serde_derive_internals, and it raises the barrier of entry extremely high (you need to have a thorough understanding of how proc macros work).

If a tiny refactoring like this is too much, how do you propose to add any way for plugins to operate? This commit adds 7 public functions; all but one is just a getter/setter, and even that one is based entirely on code and functionality that already existed.

@dtolnay
Copy link
Member

dtolnay commented Dec 2, 2018

I am suggesting that someone with more bandwidth should implement a library similar to the functionality of serde_derive_internals, but designed to be used by serde_derive plugins.

Regarding #1427, the invocations would initially look like some variation of:

#[serde_prefix_all = "NetworkResult_"]
#[derive(Deserialize)]
enum NetworkResult {
    Success,
}

which I believe is sufficient for making progress and proving out the model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants