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

Cleanup: StationCargoList::AreMergable doxygen comment references Veh… #8185

Merged
merged 1 commit into from Jun 5, 2020

Conversation

techgeeknz
Copy link
Contributor

…icle instead of Station.

There is some contention about whether this function's comment should reference Vehicle or Station. Someone who is familiar with the cargodist codepaths should review this before merging it to trunk.

@techgeeknz techgeeknz force-pushed the master_docfix_cargopacket branch 4 times, most recently from 283c656 to b96c4f2 Compare June 4, 2020 11:02
@techgeeknz
Copy link
Contributor Author

Sorry about all the force-push spam. I'm still quite new to git, and was trying to rebase the commit in such a way that it will apply cleanly to master without conflicting with the commit in pull request 8184 that I split it from.

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

The change is fine as-is and clearly just a copy-paste mistake.

@techgeeknz
Copy link
Contributor Author

Thanks for verifying my hunch, @nielsmh

@FLHerne
Copy link
Contributor

FLHerne commented Jun 5, 2020

Nitpicking: Git has a limited length (72 chars, IIRC) for the commit title, and wrapping it with "..." halfway through a word mid-sentence like Github does isn't ideal.

Could you please try to fit a basic description inside the title, or at least split it at a point that makes sense?

(this applies to a few of your recent PRs/commits)

@LordAro
Copy link
Member

LordAro commented Jun 5, 2020

It's only a GitHub display limit, and as far as I'm aware git only "recommends" a 72 char line limit. Looking at the commit log in git itself shows no weird line wrapping.

Not that you shouldn't try to keep the lengths under that, of course...

@LordAro LordAro merged commit 937b366 into OpenTTD:master Jun 5, 2020
@techgeeknz
Copy link
Contributor Author

I'll try to keep that in mind; still learning how to use git in the most effective way.
When entering a multiline description using git commit, is it correct that the first line becomes the title?

@nielsmh
Copy link
Contributor

nielsmh commented Jun 6, 2020

Yes the first line (terminated by line break) becomes the commit title. Sometimes the best is to just give a generic commit title and explain the changes in details in the rest of the description, instead of trying to cram too much information into the first line.

@techgeeknz
Copy link
Contributor Author

Thanks. I had sorta figured that out; but it was only recently I figured out how to give git a multiline commit message (I had been using the -m parameter to supply a message directly on the command line; now I use the interactive editor).

@techgeeknz techgeeknz deleted the master_docfix_cargopacket branch June 7, 2020 20:36
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.

None yet

4 participants