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

Match semver crate examples' styling #315

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@convoliution
Copy link
Contributor

convoliution commented Oct 4, 2017

At the risk of bikeshedding, I revised the styling I used for #305 to match the styling used in #308.

Probably not even necessary, though. Feelings will not be hurt if the request is closed :')
(should I have opened an issue before PR'ing?)

@budziq
Copy link
Collaborator

budziq left a comment

thanks @mykalu! two minor suggestions.

And you are welcome to update the other example to conform with this one.

src/app.md Outdated
release.increment_patch();
assert_eq!(release, Version::parse("0.2.7")?);
assert_eq!("0.2.7", release.to_string());

This comment has been minimized.

@budziq

budziq Oct 4, 2017

Collaborator

If I were to mark my preference here I would suggest putting the hardcoded string against we are testing on the right.

I probably liked your original version better but it's a matter of taste, so I'll let you chose.

src/app.md Outdated
pre: vec!(),
build: vec!(),
});
let version_str = "0.2.6";

This comment has been minimized.

@budziq

budziq Oct 4, 2017

Collaborator

i see no reason for separate version_str.

This comment has been minimized.

@convoliution

convoliution Oct 4, 2017

Author Contributor

Hmmm you're right; I separated it to match the other example, but there's no practical reason for it in this example.

@convoliution convoliution changed the title Cleanup 'Parse and increment version string' styling Match semver crate examples' styling Oct 4, 2017

@convoliution

This comment has been minimized.

Copy link
Contributor Author

convoliution commented Oct 4, 2017

Suggestions implemented!

Hmm, I'm using
assert_eq!(release.to_string(), "0.3.0");
but the Complex Version example is using
let serialized_version = parsed_version.to_string();
assert_eq!(&serialized_version, version_str);

I feel that's a styling mismatch.
But to create intermediate variables in Simple Version would be excessive,
and removing the intermediate variable in Complex Version would remove the clarity that we're checking serialization.

Also concerned about the nomenclature difference between using release vs parsed_version as the variable names.

@FaultyRAM do you have any thoughts on this? :D

@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 4, 2017

I would say that clarity+conciseness of given examples are more important tan total stylistic regime. Also I feel that parsed_version is more informative than release

@convoliution

This comment has been minimized.

Copy link
Contributor Author

convoliution commented Oct 4, 2017

Suggestions implemented!

@budziq budziq merged commit 5d41607 into rust-lang-nursery:master Oct 4, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 4, 2017

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.