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

proposal: checkstyle (& maven central) #50

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

bfu4
Copy link

@bfu4 bfu4 commented Jun 25, 2021

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
Copy link
Collaborator

rnicoll commented Jul 2, 2021

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

Copy link
Collaborator

@rnicoll rnicoll left a comment

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.

apply suggestion for documentation comment for AbstractDogeParams#getChainID

Co-authored-by: Ross Nicoll <[email protected]>
@bfu4
Copy link
Author

bfu4 commented Jul 5, 2021

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.

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