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

remove unused label [subset]RankHint from gameswidget #717

Conversation

GrotheFAF
Copy link
Contributor

There are 2 RankHint labels with slightly different text, only one 'subsetRankHint' was used. I removed the the older unused "RankHint" and renamed 'subsetRankHint' to 'RankHint'.

  • PR branch should be named issuenum-fix/feature/cleanup-description
  • Code is split into logical commits
  • Code has tests
  • Final commit includes "Fixes #issue" in commit message

When all builds pass and a maintainer is happy with your PR, the "ready" label will be applied. Please complete these tasks then:

  • Rebase onto develop
  • Add changelog entry
  • Remove this entire template section

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 22.249% when pulling 90977a1 on GrotheFAF:remove-unused-label-subsetRankedHint-Play into 1d9a630 on FAForever:develop.

@Wesmania
Copy link
Contributor

I like it. It sucks that designer is so git-unfriendly - it seems it randomly decided to remove a single space everywhere, replacing 600+ lines. I don't know if it'd be more of a pain to roll with a full replacement this once and clobber commit history, or to manually fix commits (doesn't seem especially bad in this case, we can script adding a single space everywhere).

@Wesmania
Copy link
Contributor

I tested it a tiny bit - it seems the actual problem is that qt designer decided it likes 1-space indentations rather than 2-space ones,and that causes changes everywhere. I have absolutely no idea if it's configurable, or if our .ui files are even consistent about it. If it turns out we need to change that indentation, it might be best to split this commit into one that only changes indentation and one that makes meaningful changes.

@Wesmania
Copy link
Contributor

I did some more checking - all the ui files except for this file use 1-space indents. We definitely want to bring it in line with the rest - maybe split the indent change to its own commit, so we can more easily see what meaningful changes are here. The indent thing is also worth putting on the wiki.

@Wesmania
Copy link
Contributor

Rebase on develop (+ changelog), maybe add a separate commit just for indent change, and we'll merge it in.

@Wesmania Wesmania added this to the 0.12.x milestone Apr 30, 2017
@GrotheFAF GrotheFAF force-pushed the remove-unused-label-subsetRankedHint-Play branch from 90977a1 to d31eb55 Compare April 30, 2017 21:06
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 22.539% when pulling d31eb55 on GrotheFAF:remove-unused-label-subsetRankedHint-Play into 1316de7 on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the remove-unused-label-subsetRankedHint-Play branch from d31eb55 to 213c8d7 Compare April 30, 2017 21:16
@GrotheFAF
Copy link
Contributor Author

GrotheFAF commented Apr 30, 2017

rebased and add changelog

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 22.539% when pulling 213c8d7 on GrotheFAF:remove-unused-label-subsetRankedHint-Play into 1316de7 on FAForever:develop.

@Wesmania Wesmania merged commit 213c8d7 into FAForever:develop Apr 30, 2017
@GrotheFAF GrotheFAF deleted the remove-unused-label-subsetRankedHint-Play branch April 30, 2017 21:41
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.

3 participants