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

used char instead of a single character string #194

Merged
merged 1 commit into from
Dec 21, 2017
Merged

used char instead of a single character string #194

merged 1 commit into from
Dec 21, 2017

Conversation

sdutry
Copy link
Member

@sdutry sdutry commented Dec 20, 2017

Fixes sonar reported issue about single character strings being used inside the struts2 core module.

Sonar reported issue:
Sonar - char instead of string

@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage remained the same at 46.248% when pulling fb23b71 on sdutry:sonar/string-optimize-single-characters into c4bd87e on apache:master.

@lukaszlenart
Copy link
Member

👍

@lukaszlenart lukaszlenart merged commit eddab32 into apache:master Dec 21, 2017
@rgielen
Copy link
Member

rgielen commented Dec 21, 2017

From the peanut gallery:

If I'd stumble about this commit later, I would ask myself if the message describes the problem this commit fixes or if it describes what the contributor did before submitting it as a commit. That is: "used char instead of a single character string [before this commit, now fixed]" vs "used char instead of a single character string [and then committed this as an improvement]"

Over the last years I came to the conclusion that most Git commit message conventions make perfect sense, especially rule 5 in https://chris.beams.io/posts/git-commit/ - use imperative mood for your summary, such that it completes the sentence "If applied, this commit will [your subject line here]"

When applied, the commit message would read "use char instead of a single character string" (I have no strong opinion on whether to start with a capital letter). IMO this message would be unambiguous and clear. WDYT?

To be clear, this is just for future consideration, no intention to change that now merged commit anyhow!

@yasserzamani
Copy link
Member

👍 I agree with you and will follow all of that article.

I could not find in that any thing about when several commits are related to same issue. Should we use See also: WW-#### in body of each one? In which one's body we should use Resolves: WW-####? Or maybe we should not use them in anyone and instead, always use PR and it's merge commit as an aggregator and parent?

@sdutry
Copy link
Member Author

sdutry commented Dec 21, 2017

@rgielen
I'll try and keep it in mind.

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.

5 participants