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

Implement display-bound attribute for derive-Display macro (#93) #97

Merged

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Oct 23, 2019

Second half of #93. This PR is based on top of #95.

Implemented [display(bound = "...")] attribute for Display macro as proposed by @tyranron in #93.

Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I've squashed your other PR and merged in into master. Can you update this branch to use master as a base (git cherry-pick should do it).

doc/display.md Outdated Show resolved Hide resolved
tests/display.rs Outdated Show resolved Hide resolved
@ffuugoo ffuugoo force-pushed the add-derive-display-bound-attribute branch from 340e37a to c124e84 Compare October 29, 2019 04:48
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Oct 29, 2019

I've squashed your other PR and merged in into master. Can you update this branch to use master as a base (git cherry-pick should do it).

Done.

@ffuugoo
Copy link
Contributor Author

ffuugoo commented Oct 29, 2019

@tyranron

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ffuugoo at this point consider to work well on a code style of the implementation, until it growth too complex and messy for other contributors.

src/display.rs Outdated
},
nested: inner_meta,
..
}) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this nested beast could be rewritten as we've discussed.

src/display.rs Outdated
TraitBoundModifier,
Type,
TypeParamBound,
TypePath,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why import all this? Is using syn::Type or syn::TypeParamBound is too bad and requires syn:: part to be removed?

This has sense if there is a really long paths, to shorten it for better readability. The punctuated::Punctuated is a good example. However, in much cases syn::Something is quite good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just followed existing style here. I'm for explicitly prefixing syn:: personally.

Copy link
Collaborator

@tyranron tyranron Oct 29, 2019

Choose a reason for hiding this comment

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

@JelteF wouldn't you mind on some cosmetic refactoring for src/display.rs file?

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good for the changes made here, but if it touches code in another part of the file let's do that in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ffuugoo ☝️

@@ -99,6 +114,50 @@ pub fn expand(input: &DeriveInput, trait_name: &str) -> Result<TokenStream> {
})
}

fn trait_name_to_attribute_name(trait_name: &str) -> &'static str {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider to use smart well-known abbreviations, like attr instead attribute.

tests/display.rs Outdated Show resolved Hide resolved
src/display.rs Outdated Show resolved Hide resolved
src/display.rs Outdated Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Oct 30, 2019

That's what you get when you use nightly by default. Should be fixed now.

doc/display.md Outdated Show resolved Hide resolved
@JelteF
Copy link
Owner

JelteF commented Oct 31, 2019

I think it's good now. It's still a draft PR. Is that on purpose? If no, I will merge this PR.

@ffuugoo ffuugoo changed the title WIP: Implement display-bound attribute for derive-Display macro (#93) Implement display-bound attribute for derive-Display macro (#93) Oct 31, 2019
@ffuugoo ffuugoo marked this pull request as ready for review October 31, 2019 07:54
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Oct 31, 2019

I think it's good now. It's still a draft PR. Is that on purpose? If no, I will merge this PR.

Undrafted. (:

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

I'd like to refactor it more deeply, but it's still relatively okay though.

@JelteF JelteF self-requested a review November 2, 2019 13:23
Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

LGTM. If you feel like refactoring some more feel free to make a PR. For me the main important thing would be using early returns in the whole file more often. There is some very deeply nested code.

@JelteF JelteF merged commit 20fa610 into JelteF:master Nov 2, 2019
@tyranron tyranron deleted the add-derive-display-bound-attribute branch November 2, 2019 13:40
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.

3 participants