Some of you might have already noticed, there is now some wizardry going on when a PR is created or updated. There is now a CI build server which does a “mvn test” for every PR.
The new development workflow is:
- change code
- do a local “mvn clean test” or “mvn test”
- if it builds successfully, commit, push to GitHub as usual, if not fix it and go back to 2. (deleting or disabling unit tests is NOT a fix)
- create a PR (if it’s not a commit into an existing PR)
- on GitHub, look at your PR, scroll to the bottom, there will be a result of the CI build about 1-2 minutes after the PR was created/updated
- if the result is green, all is good (e.g. github.com/vassalengine/vassal/pull/47)
- if the result is red (e.g. github.com/vassalengine/vassal/pull/46)
[list=a]
[*]there is a “Details” link in the bottom area, click on it
- on the details page is a link to the build where it says “The build failed” (e.g. github.com/vassalengine/vassal/ … =813206570), click on that link
- read the build log from bottom to top, find out what broke the build (e.g. travis-ci.com/github/vassalengi … 71847#L424)
- fix the build
- repeat from step 1
[/*:m][/list:o]
The build will currently break if:
- you introduce compile errors
- you change code in a way that unit tests fail
Coming soon:
- build will break when the code is not properly formatted
- build breaks when the binary compatibility changes i.e. custom code from modules becomes incompatible
And some more goodies in the pipeline:
- use maven to build the linux release - about complete
- use maven to build the windows release - can build the VASSAL.exe already, next step is the NSIS
- use maven to build the macosx release - 0%
I noticed yesterday it’s only running tests on my newer PR’s. Any action needed on PR’s from several days ago? Or just let them be?
Thus spake Cattlesquat:
I noticed yesterday it’s only running tests on my newer PR’s. Any action
needed on PR’s from several days ago? Or just let them be?
To trigger Travis, you’d need a new commit. I test PRs locally anyhow, so
no need to do anything for the outstanding pre-Travis ones (which I am
slowly getting to…).
–
J.
Gentlemen, we now have a thing called checkstyle (github.com/checkstyle/checkstyle) as part of the CI build pipeline.
Right now the build is broken due to too many INFO-level notifications ending up in the build log, but this should be fixed soon and from then on, please do the following:
- if you are bored, scan the build log for checkstyle warnings, fix them and make a PR
- if you change code, please try not to introduce new warnings
For Eclipse users: your maven plugin might (mine did when I tested this) act up the next time you refresh your project configuration and propose to install the checkstyle plugin. A half hour later the plugin should be installed and your local build log in Eclipse will be filled with checkstyle warnings. If you can be bothered, remove as many as you can.
Once we get rid of all current warnings, we will raise the severity level for these types of formatting issues to ERROR, from then on the build will break if these formatting rules get violated. At the same time the severity level of the next set of formatting rules will be raised from INFO to WARN.
Currently, violations of the following formatting rules are reported as warnings (see src/test/resources/checkstyle-checks.xml):
- FileTabCharacter: usage of tabs instead of spaces
- LeftCurly: non-C++ / Java-style placement of {, we expect it to be on the same line of the preceding expression
- RightCurly: } should be placed alone on its line in most cases
- Indentation: indentation by 2 spaces (case statements indented by 0 spaces)
Yes, this also means that things like
public void getField() { return field; }
are history, these need 3 lines of code now, the trick here is to have a cool modern IDE that shows such methods as if they were written on a single line, e.g.
[attachment=0]code_collapsed.png[/attachment]
Thus spake Flint1b:
Yes, this also means that things like
Code:
public void getField() { return field; }
We need to adjust the rules to accept this.
–
J.
Single-line methods are no good, for several reasons:
- to someone who knows what he is doing, it doesn’t matter whether single-line methods are written on a single line or not
- generally, it is not acceptable nowadays to have more than one statement per line (this also includes multiple variable definitions like “int x, y;”)
- when debugging, with the single-line version it is impossible to differentiate between a breakpoint on a method or on the single statement
- when working and trying to understand the code, and also when debugging, it is much easier to add code e.g. a logging statement to the expanded version
- saving disk space is nowadays not done by by removing a couple newline characters, these few bytes don’t matter
- code reviews and reporting tools can handle the code much easier when each statement is in its own line, with the single-line version it’s impossible to communicate sth like “the code in line 47 is not that good and could be improved” - what exactly would this mean in the case of a single-line method, the method signature or the single statement inside the method?
- probably around half of the typical Java code (Vassal is not typical) is getters and setters with a single statement per method, it never was the general practice to collapse them into single lines, and never will be
- a good modern IDE will make single-line methods and also other constructs like anonymous types appear as collapsed for easier reading, and no, edlin/edit.com/borland’s 80x25 yellow-on-blue/vi/emacs/eclipse are not good and modern editors for Java
Java and modern coding practices in general are often times verbose, but we are not aiming to win the obfuscated C code contest are we?
Incidentally, many code styles go back to demanding a maximum line length of 80 columns, the idea is to make it possible to view diffs side-by-side on a single screen or projected onto a wall without having to scroll horizontally. Part of achieving such a short line length is to never write methods in a single line.
Also, why are C++ developers calling methods “functions”, there are no functions at all in Java, it’s all methods, and I thought it was the same in C++ with the methods of a class? Or is this a legacy of C++ not being a real language at all just a set of macros around C, and C++ classes not being proper language structures either, just some dirty hacks with C structs and passing the pointer to the struct as an invisible argument in class methods? 
In C it was nothing but functions. And so when classes appeared with C++ those who were learning about that stuff from non-academic sources (i.e. learning on the job) just saw classes as “structs that contain functions”. The official terminology came in gradually behind it, but the legacy remains. To this day my brain’s definition of “method” is “a function that’s a member of a class” (at least I don’t think of functions as “a special kind of subroutine” any more).
The first PR that enforces certain formatting rules is getting closer to be merged into master. After some reconsiderations and adjustments, this is the final list of rules that are enforced now, violations of these rules will break the build if they are encountered:
- tab characters encountered anywhere in the code
- what is called RightCurly in checkstyle, the “}” thingie, must be alone in it’s line, when it is part of this list of structures: CLASS_DEF, CTOR_DEF, LITERAL_FOR, STATIC_INIT, INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF, LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE, LITERAL_WHILE
- indentation, the offset is two, except for case statements which must have an offset of zero, i.e. must be aligned under their switch statement
What would be a good next set of formatting rules to apply? Here are some suggestions, we can also browse checkstyle-checks.xml for the full list:
- WhitespaceAfter - enforce whitespace after COMMA, SEMI, TYPECAST, LITERAL_IF, LITERAL_ELSE, LITERAL_WHILE, LITERAL_DO, LITERAL_FOR, DO_WHILE, or only some of them
- WhitespaceAround - enforce whitespace around various literals, the full list possible literals is too long to paste it here but you get the idea, we all know how ugly x*3+y+5 looks in code
- NoWhitespaceBefore - no whitespace before COMMA, SEMI, POST_INC, POST_DEC, DOT, LABELED_STAT, METHOD_REF
- MemberName - proper names for class members (camelCase with 1st lowercase is proper, this would catch all the remaining c-style variable_names)
- ParameterName, LambdaParameterName, CatchParameterName, LocalVariableName, MethodName, same as above for some or all of these 5 types of names
Also: we have exactly three and only these three options for enforcing the LeftCurly “{” in method definitions:
- same line i.e. C+±style
void aMethod(int param) {
...
void aMethod(int param 1, int param2,
int param 3) {
- on its own line i.e. C-style
void aMethod(int param)
{
...
void aMethod(int param 1, int param2,
int param 3)
{
- give up automatic enforcement of this and come up with some unnecessarily complex rules for when it’s C++ style and when it is C style, then have a human tediously check this in every PR (Joel I thought you are a busy man and want to spend more time on the new Vassal?)
I personally am for either 1. or 2., main point here is to get the dumb computer to check this and relieve a human from this tedious and in 2020 unnecessary work.
So I personally loathe tabs, but I feel like I find files that are rife with them, and they are very difficult to coax software into removing.
I guess if that started getting enforced but all the files we pulled down were guaranteed to not have tabs in the first place and we were just responsible for not inserting any MORE then that’s fine.
Yes this would be the workflow, we (I, personally, if needs be) remove existing formatting errors, from then on the code is guaranteed to not have any and we are just responsible for not inserting new ones.
Thus spake Cattlesquat:
So I personally loathe tabs, but I feel like I find files that are rife
with them, and they are very difficult to coax software into removing.
I thought I had replaced all the tabs at one point long ago, but maybe
what I did instead was replace all the tabs which didn’t involve a lot
of reformatting.
–
J.
As long as there is no automated process that ensures a set of formatting rules, these manual adjustments are a waste of time.
Gentlemen, can we agree on a set of formatting rules that is maybe not perfect but good enough, and most importantly can be enforced by the computer during the build process, and forget this topic once and for all? This is what all this checkstyle stuff is about, to free up valuable time that we currently spend shoving the PRs back and forth due to code formatting. There is enough work to do, we should be working on important matters, not on the placement of braces.
Can we decide where we want our left curly brace {, same line or in it’s own line? Checkstyle is too dumb for complex rules like "if its a method with parameters spanning multiple lines then do it like this and otherwise do it like that 
It certainly seems like it will have lots of advantages once we get “over the hump” of adoption.
Also it will spare me perpetual public shaming because I’m guaranteed to type } else { 90% of the time unless a machine makes me stop. So there’s that.
Thus spake Flint1b:
Gentlemen, can we agree on a set of formatting rules that is maybe not
perfect but good enough, and most importantly can be enforced by the
computer during the build process, and forget this topic once and for
all? This is what all this checkstyle stuff is about, to free up
valuable time that we currently spend shoving the PRs back and forth due
to code formatting. There is enough work to do, we should be working on
important matters, not on the placement of braces.
Can we decide where we want our left curly brace {, same line or in it’s
own line? Checkstyle is too dumb for complex rules like "if its a method
with parameters spanning multiple lines then do it like this and
otherwise do it like that
It’s a quality of life issue for me. It is intensely irritating to me
to have a checker which isn’t sufficiently nuanced to handle the desired
formatting. It makes the code harder to read, and it pisses me off every
single time I look at it. It’s like having a rock in my shoe that I can’t
get rid of.
I will very grudgingly accept it here, but don’t expect me to ever be
happy about it.
–
J.
Oh come on Joel, just think of the amount of time that is freed up by this, time that you can spend on more important things, family, kids, playing boardgames, or the currently main mission of your life, Vassal 4/C++/NG development 
Thought I’d append this in this topic as it is about the workflow.
The first big code formatting change is merged, from now on the build will break when certain formatting rules are violated.
Currently existing PRs should merge the master branch and resolve possible conflicts, I think I’ve seen at least several PRs that did the same code formatting fixes that I did in that merged PR.