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

Checkstyle: Fix line length violations (Part 2) #1638

Conversation

ssoloff
Copy link
Member

@ssoloff ssoloff commented Apr 12, 2017

This PR fixes violations of the Checkstyle LineLength rule.

I'm splitting fixes for the LineLength rule into multiple PRs because, while the number of changes is not particularly large, they do require a bit more scrutiny. So I'm trying to keep the PR smaller to not overwhelm the reviewer.

@simon33-2
Copy link
Contributor

simon33-2 commented Apr 13, 2017

Can I suggest some better things to do:

  • Fix the bugs in sub based combat
    • If the side being fired on has a destroyer, they shouldn't be asked for casualties until they know the number of hits from all units
    • if the side being fired on doesn't have a destroyer and the side firing has air, the side being fired on should know the number of hits from all units rather than needing to select air casualties first (or something like that)
  • Auto place carriers and update the prompt for when air is moved in non combat to a sea zone with a an industrial complex and a carrier bought to be "Keep Moving"/"Place Carrier(s)" rather than "Keep Moving"/"End Move Phase". This needs to remove the carriers placed from the I guess there is a third possibility where you have some air that can be caught by a mobilising carrier and some which will be stranded. In that case I suggest that the new prompt is displayed, and then the old one.
  • If there is only one unit to place in the unit picker, don't ask the user
  • Scramble box - don't allow them to select scramble with 0 units selected. Similarly with vice versa.
  • If there is only one potential unit placement location, auto place.
  • Merge purchase and repair units - this is a rule violation at present. I have a suspicion that this can be done without an engine change but I can't see it? Best option IMO would be to have three buttons on the panel - buy, repair and done. So a single "done" for the whole phase.
  • Allow repair of disabled facilities without edit for maps with combat move before purchase
  • If you move a plane in combat movement such that the only landing space is a newly mobilised carrier, if one isn't already purchased (such as if Combat Move is before purchase), ask to purchase the CV if the map has a newly mobilised carrier as a valid landing spot and the player has sufficient resources. This shouldn't be undoable in purchase although it should be undone if the move is undone.
  • Update the battle selection box to disable battles with a dependency until the dependency is dealt with.
  • Only amphibious assaults and their dependent battles should be selectable until all the amphibious assaults are dealt with, or prevented with a sea battle.

How's that for a list? Biggest priority would be the sub logic although most of the others are much easier to achieve.

@ssoloff
Copy link
Member Author

ssoloff commented Apr 14, 2017

@simon33-2 That looks like a good selection of features and fixes, and I'd be happy to contribute to some of them. In fact, a few look like good candidates for me to increase my knowledge of game mechanics. Are there already open issues for any of them? Is so, could you edit your comment and reference the issue number? I'll then bookmark the comment for later review.

However, I plan on starting to prototype the serialization proxy stuff this weekend, so, other than dropping a few more Checkstyle PRs for the remaining no-risk and low-risk violation fixes, I probably won't get to any other meaningful changes in the near future to avoid context switching.

@ssoloff ssoloff force-pushed the checkstyle-fix-line-length-violations-part-2 branch from 2b591cb to 6906866 Compare April 14, 2017 06:02
@ssoloff
Copy link
Member Author

ssoloff commented Apr 14, 2017

Rebased against master at 240e942 (1.9.0.0.4043).

@simon33-2
Copy link
Contributor

Not all of those have issues open.

@ron-murhammer ron-murhammer merged commit f632f85 into triplea-game:master Apr 15, 2017
@ssoloff ssoloff deleted the checkstyle-fix-line-length-violations-part-2 branch April 15, 2017 17:51
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