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

#6315 Rail fences in snow or desert #7029

Closed
wants to merge 7 commits into from
Closed

Conversation

@pi1985
Copy link

pi1985 commented Jan 8, 2019

No description provided.

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jan 8, 2019

Sorry, this is not going to work. The commits should work on their own, in other words you should be able to compile every revision and have a playable game in every instance. I know it's somewhat double work, but expressing the change as first a part that restructures the existing code without changing the user-visible functionality, and then second part adds the new feature, makes it easier to follow.

You do not need to open a new PR for having made complete restructuring. GitHub responds correctly to a force-push to a pull request branch, replacing all the proposed commits with the new set.

The commit checker is also still failing because of some whitespace at end of line:

*** b/src/rail_cmd.cpp:2014: Trailing whitespace: '		// Track bit on this edge => no fence. '
*** b/src/rail_cmd.cpp:2019: Trailing whitespace: '		// Show fences if it's a house, industry, object, road, tunnelbridge or not owned by us. '
*** b/src/rail_cmd.cpp:2785: Trailing whitespace: '						'
*** b/src/rail_cmd.cpp:2810: Trailing whitespace: '			'
*** b/src/rail_cmd.cpp:2816: Trailing whitespace: '			'
*** b/src/rail_cmd.cpp:2822: Trailing whitespace: '			'`
Copy link
Member

LordAro left a comment

Commit checker is still very unhappy, I'm afraid

 *** b/src/rail_cmd.cpp:2014: Trailing whitespace: '		// Track bit on this edge => no fence. '
 *** b/src/rail_cmd.cpp:2019: Trailing whitespace: '		// Show fences if it's a house, industry, object, road, tunnelbridge or not owned by us. '
 *** b/src/rail_cmd.cpp:2785: Trailing whitespace: '						'
 *** b/src/rail_cmd.cpp:2810: Trailing whitespace: '			'
 *** b/src/rail_cmd.cpp:2816: Trailing whitespace: '			'
 *** b/src/rail_cmd.cpp:2822: Trailing whitespace: '			'
 *** b/src/water_cmd.cpp:1121: Invalid tab usage: '/*			switch (GetTrackBits(tile)) {'

Additionally, it looks like you've duplicated commits, instead of rebasing them together like how we'd prefer it - each commit should be a logical change, usually structured in such a fashion that refactors in preparation for features come first, then the feature is implemented in a separate commit.

Finally, the SAVEGAME_VERSION constant is now out of date, and missing a comment with the PR number (no idea why that isn't causing conflicts...)

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Mar 3, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@TrueBrain TrueBrain added the stale label Mar 3, 2019
@stale stale bot closed this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.