Skip to content

STORM-3417: Fix checkstyle violations in sql-core#3035

Merged
srdo merged 1 commit intoapache:masterfrom
krichter722:checkstyle-sql-core
Jun 27, 2019
Merged

STORM-3417: Fix checkstyle violations in sql-core#3035
srdo merged 1 commit intoapache:masterfrom
krichter722:checkstyle-sql-core

Conversation

@krichter722
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@srdo srdo left a comment

Choose a reason for hiding this comment

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

+1 assuming tests pass, left a question but it's not very important.


/**
* @return Whether compilation was successful.
* Compiles source code to byte code.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wasn't this check disabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We removed the rule to add missing Javadoc on public methods and classes. The rules to validate already present Javadoc were present and remained untouched. I think this makes sense because if Javadoc is present it should be valid. It could also be removed but deciding that slows down the PR workflow in my opinion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks.

@krichter722
Copy link
Copy Markdown
Contributor Author

@srdo This can be merged, right?

@srdo
Copy link
Copy Markdown
Contributor

srdo commented Jun 26, 2019

@krichter722 Yes, soon. We generally wait at least 24 hours before merging, so others get a chance to review.

@srdo srdo merged commit 4822ecd into apache:master Jun 27, 2019
@krichter722 krichter722 deleted the checkstyle-sql-core branch June 27, 2019 19:29
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