Skip to content

proposal: checkstyle (& maven central)#50

Draft
bfu4 wants to merge 6 commits into
dogecoin:mainfrom
bfu4:checkstyle
Draft

proposal: checkstyle (& maven central)#50
bfu4 wants to merge 6 commits into
dogecoin:mainfrom
bfu4:checkstyle

Conversation

@bfu4

@bfu4 bfu4 commented Jun 25, 2021

Copy link
Copy Markdown

What's new

  • Added a checkstyle file.
  • IntelliJ also formatted 2 files, and with that I documented 2 methods.

Reasoning

I was browsing through Github and came upon this repository. This is quite cool. This repository is also already ery well documented (yay!). I'm suggesting a checkstyle to enforce maintenance on that, as well as just to have the feature to check the style of future additions. As the checkstyle is very strict, the code does not compile due to a few missing documentation features. This can be checked out by pulling the branch and running any of the checkstyle tasks for the projects that include Java files (all projects except wallettemplate).

The checkstyle follows a template used by Sun, and also used by hyperledger's sdk, except is modified to allow the ternaries and magic numbers.

I'd love to be able to contribute to this, but I'd not go forth with doing the documentation without pitching it here. I'm creating this pull request to see if this is something that would like to be done and if so, track progress and discussion here. With that, this PR is marked as a draft 😃.

The checkstyle excludes the file generated by the protobufs.

Other Notes

Eventually, I think #28 has a good point. This requires credentials but could be done as well. Of course, I cannot implement that, as it should be published under dogecoin's group.

I know the pain of checkstyles, so I'm sure seeing a pull request adding a checkstyle is probably nothing short to painful, especially for a repository with so many files and is already very well documented. However, I think with something like libraries, the possibility of contributions can be greeted with enforcing a style followed throughout the project, as well as is just a nice feature to have.

Since this repository is not actively maintained (it doesn't seem to be, at least) this PR can be viewed as more of a casual suggestion, and if it came off as more harsh (it being a checkstyle, and all), it did not mean to be.

@rnicoll

rnicoll commented Jul 2, 2021

Copy link
Copy Markdown
Collaborator

Sorry, missed that this was a PR not an issue, had scheduled this for when I had time to write code. Will review ASAP.

@rnicoll rnicoll left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Couple of minor tweaks (I've raised suggestions, so should be easy to do), but generally I think looks good.

Comment thread core/src/main/java/org/libdohj/params/AbstractDogecoinParams.java Outdated
Comment thread core/src/main/java/org/libdohj/params/AbstractDogecoinParams.java Outdated
apply suggestion for documentation comment for AbstractDogeParams#getChainID

Co-authored-by: Ross Nicoll <rnicoll@rnicoll.name>
@bfu4

bfu4 commented Jul 5, 2021

Copy link
Copy Markdown
Author

Awesome! I'm going to commit these documentation suggestions now, and update some of the code to be compliant with the checkstyle. After those, there are about 88 missing comments (in the core module), where most should be relatively easy to add.

bfu4 and others added 4 commits July 4, 2021 23:45
update documentation comment per suggestion for AbstractDogecoinParams#getSerializer

Co-authored-by: Ross Nicoll <rnicoll@rnicoll.name>
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.

2 participants