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.

Sign up for GitHub

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

Jump to bottom

Add 'Code Style and Quality Guide' #224

Closed
wants to merge 3 commits into from
Closed

Add 'Code Style and Quality Guide' #224

wants to merge 3 commits into from

Conversation

rmetzger
Copy link
Contributor

@rmetzger rmetzger commented Jul 8, 2019 •
edited
Loading

sjwiesman
sjwiesman requested changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@sjwiesman sjwiesman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for driving this @rmetzger , found a few nits but otherwise looks good. The third page is long but I think that's fine.

contributing/code-style-and-quality-preamble.md Outdated Show resolved Hide resolved
contributing/code-style-and-quality-general.md Outdated Show resolved Hide resolved
contributing/code-style-and-quality-general.md Outdated Show resolved Hide resolved
@fhueske fhueske mentioned this pull request Jul 10, 2019
Closed
@rmetzger
Copy link
Contributor Author

Thanks for your review @sjwiesman. I addressed your comments.

zentol
zentol requested changes Jul 15, 2019
View reviewed changes
contributing/code-style-and-quality-general.md Outdated
### Use inspections in IntelliJ

* Import the inspections settings into the IDE (see IDE setup guide)
* TODO: Need to agree on a profile and export it (like checkstyle)
Copy link
Contributor

Choose a reason for hiding this comment

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

our docs shouldn't contain TODOS

Copy link
Contributor

Choose a reason for hiding this comment

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

I would throw out this entire section because it isn't remotely well-defined

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we comment this out for now?
I think we should agree upon a set of inspections for all committers/contributors to use. That would help a lot in my opinion.

I'd volunteer with a first suggestion of inspections.

contributing/code-style-and-quality-pull-requests.md Outdated
Exceptions are ****hotfixes****, like fixing typos in JavaDocs or documentation files.


Name the pull request in the form `[FLINK-XXXX] [component] Title of the pull request`, where `FLINK-XXXX` should be replaced by the actual issue number. The components should be the same as used in the JIRA issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

the majority of PRs/commits that I see do not have a space between ][

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Any version works, as long as we are consistent.
I am fine with "no space in between".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also see the "no space in between" variant more often. I'll change it

contributing/code-style-and-quality-pull-requests.md Outdated

Examples for changes that strictly need to go into a separate commit include


Copy link
Contributor

Choose a reason for hiding this comment

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

Please standardize the number of empty lines before lists.

contributing/code-style-and-quality-pull-requests.md Outdated
## 4. Commit Naming Conventions

Commit messages should follow a similar pattern as the pull request as a whole:
`[FLINK-XXXX] [component] Commit description`.
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, no space seems more common

@StephanEwen
Copy link
Contributor

I think the third page would be best split up into

  • common coding guide
  • Java Language issues
  • Scala Language issues
  • Component specific principles and design patterns
  • Formatting guide (we could also skip that and simply let checkstyle be the source of truth)

@rmetzger
Copy link
Contributor Author

rmetzger commented Aug 1, 2019

Thanks a lot for your suggestions and comments!

I have updated the pull request. I'll merge it by end of this week, if there's no further feedback.

@asfgit asfgit closed this in 8a173c9 Aug 7, 2019
XComp pushed a commit to XComp/flink-web that referenced this pull request Sep 7, 2020
@rmetzger @XComp
Another round of addressing comments
0e8f67f 
This closes apache#224
curcur pushed a commit to curcur/flink-web that referenced this pull request Apr 5, 2021
@rmetzger @curcur
Another round of addressing comments
e0d2ab9 
This closes apache#224
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanEwen StephanEwen StephanEwen left review comments

@sjwiesman sjwiesman sjwiesman requested changes

@wuchong wuchong wuchong left review comments

@zentol zentol zentol requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rmetzger @StephanEwen @sjwiesman @wuchong @zentol