Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API Cleanup 2 #238
API Cleanup 2 #238
Changes from all commits
ecbb7d0
621fa7e
58ae934
3884a20
12689a8
5e128d4
178f7a4
a7bca56
5b38591
75515fe
b079067
160cb4d
cab816c
e1e4907
9618b12
9139026
e7a028a
46eee1a
9531711
b29cfb8
79c6219
4198401
92ee226
a606653
2b1ed31
00b3a17
340f1ba
744d5a9
b906d24
ef79d5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my opinion it is more clear if we use ObjectStyle, especially in macros where type inspection with idea is a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of disagree here. It's a standard pattern. See the documentation and an example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I can find my way in. In this case, we have a macro for which we don't have a type of inspection or highlighting for a lot of IDEs. ObjectStyle::default()
is much more specific than
Deafault::default()`, so someone who reads this macro can better understand what it says. So it's more about readability and being specific in macros.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro looks like this ...
... and you have the
ObjectStyle
2 lines above the..Default::default()
, which looks enough & fine to me.There's also a similar case in the macro below (
def_str_attr
) ...... where
Default::default()
is used, but there's noObjectStyle
at all. Which is, from the readability point of view a bit worse than the first case.I do not want to force my opinion here, both ways can be used, so, I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, merged while I was typing. I assume that there's no need to change it once it is already merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I am fine with it. Isn't that big of a deal. Otherwise, I wouldn't have merged it.