Skip to content

Introduce maven checkstyle plugin#329

Closed
yuokada wants to merge 5 commits intodatafaker-net:masterfrom
yuokada:ckecktyle-plugin
Closed

Introduce maven checkstyle plugin#329
yuokada wants to merge 5 commits intodatafaker-net:masterfrom
yuokada:ckecktyle-plugin

Conversation

@yuokada
Copy link
Contributor

@yuokada yuokada commented Sep 9, 2022

  • Introduce maven-checkstyle-plugin
  • Reformat
  • Reformat: fix import order

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Merging #329 (2cc0d18) into master (0e326aa) will not change coverage.
The diff coverage is 90.00%.

❗ Current head 2cc0d18 differs from pull request most recent head 02a65f4. Consider uploading reports for the commit 02a65f4 to get more accurate results

@@            Coverage Diff            @@
##             master     #329   +/-   ##
=========================================
  Coverage     94.93%   94.93%           
  Complexity     1938     1938           
=========================================
  Files           203      203           
  Lines          3866     3866           
  Branches        383      383           
=========================================
  Hits           3670     3670           
  Misses          100      100           
  Partials         96       96           
Impacted Files Coverage Δ
src/main/java/net/datafaker/EldenRing.java 100.00% <ø> (ø)
src/main/java/net/datafaker/FakeCollection.java 86.53% <0.00%> (ø)
src/main/java/net/datafaker/IdNumber.java 100.00% <ø> (ø)
src/main/java/net/datafaker/MassEffect.java 100.00% <ø> (ø)
src/main/java/net/datafaker/fileformats/Xml.java 100.00% <ø> (ø)
...main/java/net/datafaker/idnumbers/PeselNumber.java 100.00% <ø> (ø)
.../main/java/net/datafaker/service/LocalePicker.java 90.62% <ø> (ø)
src/main/java/net/datafaker/Faker.java 98.38% <100.00%> (ø)
src/main/java/net/datafaker/Mbti.java 100.00% <100.00%> (ø)
...rc/main/java/net/datafaker/service/FakeValues.java 81.91% <100.00%> (ø)
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@snuyanzin
Copy link
Collaborator

snuyanzin commented Sep 9, 2022

We are already using spotless (https://github.com/diffplug/spotless) which gives autoformatting.... while checkstyle can not do this.
Currently it is enabled only for imports and also could be enabled for other code...
So I do not see a real value to have checkstyle.... Or am I missing something?

@yuokada
Copy link
Contributor Author

yuokada commented Sep 9, 2022

@snuyanzin I didn't know about spotless itself.

Currently it is enabled only for imports and also could be enabled for other code...

As far as I checked, source files have different import order style. For example, the 2 following files have different import styles.

  • import java.time.LocalDate;
    import java.time.ZoneId;
    import net.datafaker.idnumbers.EnIdNumber;
    import net.datafaker.idnumbers.EnZAIdNumber;
    import net.datafaker.idnumbers.EsMXIdNumber;
    import net.datafaker.idnumbers.NricNumber;
    import net.datafaker.idnumbers.NricNumber.Type;
    import net.datafaker.idnumbers.PeselNumber;
    import net.datafaker.idnumbers.PeselNumber.Gender;
    import net.datafaker.idnumbers.PtNifIdNumber;
    import net.datafaker.idnumbers.SvSEIdNumber;
    import net.datafaker.idnumbers.ZhCnIdNumber;
  • import net.datafaker.service.RandomService;
    import java.text.SimpleDateFormat;
    import java.util.ArrayList;
    import java.util.Date;
    import java.util.UUID;
    import java.util.logging.Level;
    import java.util.logging.Logger;

Are these code your intent?

@snuyanzin
Copy link
Collaborator

This is because of configuration https://github.com/datafaker-net/datafaker/blob/master/pom.xml#L203-L205
it currently only removes/checks for unused imports.
To have autoformatting enabled it should be something like that

<java>
            <googleJavaFormat>
              <version>1.7</version>
              <style>GOOGLE</style>
            </googleJavaFormat>
            <removeUnusedImports/>
          </java>

however as far as I remember @bodiam had some concerns about formatting

@yuokada
Copy link
Contributor Author

yuokada commented Sep 9, 2022

I see.

however as far as I remember @bodiam had some concerns about formatting

Just my curious. I would like to know them if possible.

@bodiam
Copy link
Contributor

bodiam commented Sep 9, 2022

Oh, not real concerns, but I don't ever want to encounter a failing build or anything like it because I put a space too many in some file, or because I use a * for the imports or the other way around.

The formatting is "whatever IntelliJ's defaults are", and that's mostly it.

I've probably been bitten too much about strict styles in corporate projects which almost add no value to a project, so I guess I'm a bit put off by tools like this.

@snuyanzin
Copy link
Collaborator

snuyanzin commented Sep 9, 2022

I would say this is where spotless could help with auto formatting
if you run mvn clean install then it could fail because of styling issues
however if you run mvn spotless:apply && mvn clean install then first command will autoformat according to rules and second will do the job you want. So this combination will not fail because of styling issues, it will fix them however you still need to commit the result of this fix

@yuokada
Copy link
Contributor Author

yuokada commented Sep 9, 2022

Thank you for explanations. I understand policy for this repository.
I will close this.

@yuokada yuokada closed this Sep 9, 2022
@yuokada yuokada deleted the ckecktyle-plugin branch September 9, 2022 09:35
@bodiam
Copy link
Contributor

bodiam commented Sep 9, 2022

Not really a policy, plus we absolutely appreciate your contribution, I like to keep it as low on rules and process as possible, that's mostly it.

@snuyanzin
Copy link
Collaborator

The way we could continue is to double check if there is a way to configure google-java-format plugin in IntelijIdea and other IDEs to behave same way like it is mentioned in pom.xml.
In case of success we could enable spotless since autoformatting from spotless and IDE will the same

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.

4 participants