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

@techgeeknz techgeeknz commented Jun 4, 2020

…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 Jun 4, 2020
@techgeeknz techgeeknz force-pushed the master_docfix_cargopacket branch from b96c4f2 to 9873028 Jun 4, 2020
@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 5, 2020

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.

nielsmh
nielsmh approved these changes Jun 5, 2020
Copy link
Contributor

@nielsmh nielsmh left a comment

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

@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 5, 2020

Thanks for verifying my hunch, @nielsmh

@FLHerne
Copy link

@FLHerne 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 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
8 checks passed
@techgeeknz
Copy link
Contributor Author

@techgeeknz techgeeknz commented Jun 5, 2020

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 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

@techgeeknz techgeeknz commented Jun 6, 2020

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 Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants