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

Format Enforcement Part 1 #119

Merged
merged 2 commits into from
Jul 23, 2014
Merged

Format Enforcement Part 1 #119

merged 2 commits into from
Jul 23, 2014

Conversation

hazendaz
Copy link
Member

All formatting now consistent. Used least change approach leaving tabs
as default spacing. At this point, only turned this on at the main
project level. Will need to revisit to get it working across the entire
project in part 2. No other hidden source changes were made here
outside of the POM.XML and new FORMAT.XML files.

A little about formatting. It uses eclipse to do this but does not
require eclipse usage directly. It will pull in the necessary jars via
maven to perform formatting with using maven-java-formatter-plugin. As
the name implies, it currently only works against java code and is based
on 3.8 version of eclipse currently.

Formatting removes extra blank lines, aligns properties, and corrects
spacing across lines. Default line is now considered 120 characters
rather than 80 as most modern monitors handle this better and it is
easier to read project overall.

While the formatter will now always run, it has no real impact after
formatting is enforced other than checking that nothing needs done.
What this does for us is to ensure any new developer using the project
applies to our formatting requirements without either party having to
fix formatting related issues. This essentially occurs before even the
pull request can be made provided a build was completed first. If for
some reason we want to turn off the feature, it requires no more than
disabling the profile for format and making it manual such that we do
this ourselves. However, I do not envision that being an issue as I have
used this in a group setting over many projects for some time without
any users running into issues with it.

Once this is applied and accepted, we need to make a decision on tabs vs
spacing. Using enforced tabs does look odd in properties of classes and code
scanners do not like this practice of using tabs in general due to OS
differences. I also noticed that in some cases, it has a harder time lining up
the code in the properties area of classes as well which is not an issue with
spaces. If we go ahead and switch to spaces, the next commit will
be as large on a per file basis but much larger in overall change. With
at least this go, I wanted to not do that initially. This was more to allow a chance
to see what the formatter actually does on a leaser scale than the final run will
produce.

All formatting now consistent.  Used least change approach leaving tabs
as default spacing.  At this point, only turned this on at the main
project level.  Will need to revisit to get it working across the entire
project in part 2.  No other hidden source changes were made here
outside of the POM.XML and new FORMAT.XML files.

A little about formatting.  It uses eclipse to do this but does not
require eclipse usage directly.  It will pull in the necessary jars via
maven to perform formatting with using maven-java-formatter-plugin.  As
the name implies, it currently only works against java code and is based
on 3.8 version of eclipse currently.

Formatting removes extra blank lines, aligns properties, and corrects
spacing across lines.  Default line is now considered 120 characters
rather than 80 as most modern monitors handle this better and it is
easier to read project overall.

While the formatter will now always run, it has no real impact after
formatting is enforced other than checking that nothing needs done.
What this does for us is to ensure any new developer using the project
applies to our formatting requirements without either party having to
fix formatting related issues.  This essentially occurs before even the
pull request can be made provided a build was completed first.

Once this is applied and accepted, we need to make a decision on tabs vs
spacing.  Using tabs now enforced does look odd in properties and code
scanners do not like this practice of using tabs in general due to OS
differences.  If we go ahead and switch to spaces, the next commit will
be as large on a per file basis but much larger in overall change.  With
at least this go, I wanted to not do that initially.
private PrincipalFormat _principalFormat = PrincipalFormat.fqn;
private PrincipalFormat _roleFormat = PrincipalFormat.fqn;
private boolean _allowGuestLogin = true;
private static final Logger _log = LoggerFactory.getLogger(WindowsLoginModule.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this example is a good one for tabs vs. spaces. It looks weird because of tabs, right? So I definitely think we should go with spaces.

@dblock
Copy link
Collaborator

dblock commented Jul 21, 2014

Add to this the next step for formatting changes, I am mostly interested in the enforcing part. Right now that would run automatically, right?

Fix to pom relative locations for git build number to eliminate
duplicate plugin in demo pom and fix so any code added in demo plugin
will pick up formatting.

Formatting now changed from tabs to spaces.
@hazendaz
Copy link
Member Author

Added tab to space change. This is ready for review / acceptance.

dblock added a commit that referenced this pull request Jul 23, 2014
Format Enforcement Part 1
@dblock dblock merged commit 4391af7 into Waffle:master Jul 23, 2014
@dblock
Copy link
Collaborator

dblock commented Jul 23, 2014

Merged.

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.

None yet

2 participants