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

checkstyle with contributing guides #69

Merged
merged 1 commit into from
Aug 17, 2015
Merged

Conversation

lanwen
Copy link
Member

@lanwen lanwen commented Aug 17, 2015

no any functional change, only code style and maven-checkstyle-plugin in pom

Review on Reviewable

## Indentation

1. **Use spaces.** Tabs are banned.
2. **Java blocks are 4 spaces.** JavaScript blocks as for Java. **XML nesting is 4 spaces**
Copy link
Member

Choose a reason for hiding this comment

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

xml with 4 spaces become very wide

Copy link
Member Author

Choose a reason for hiding this comment

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

no, look at pom.

@KostyaSha
Copy link
Member

CONTRIBUTING.md first of all should contain links to jenkins documentation and jira

@KostyaSha
Copy link
Member

Test failed, master was ok

+ Change `Do not wrap` to `Wrap if long`
+ Change `Do not force` to `Always`
- Javadoc
+ Disable generating `<p/>` on empty lines
Copy link
Member

Choose a reason for hiding this comment

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

why it critical?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's ugly. We not going to generate javadoc site, so it should be readable as is. <p/> is not for reading it as is

@lanwen lanwen force-pushed the checkstyle branch 4 times, most recently from 1e91c60 to a10bf6e Compare August 17, 2015 16:37
no any functional change, only code style and maven-checkstyle-plugin in pom
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
Copy link
Member

Choose a reason for hiding this comment

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

put all namespaces on new line?

Copy link
Member Author

Choose a reason for hiding this comment

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

its autoformatting side effect. Think can be ignored

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@oleg-nenashev
Copy link
Member

👎 it causes too much merge issues for the people. The change should not enforce checkstyle for the old code (at least now)

@KostyaSha
Copy link
Member

@oleg-nenashev other PRs (except workflow) are not actual and workflow can be easily updated ourself. Coding style is really required for comfort development and bugs reduction, so it need to be merged. Also no back-porting in this plugin expected (for hotfix i preserving minor version now).
If you mean your FB PR, then we can block this PR, but please end FB PR faster. Will it work for you?

@lanwen
Copy link
Member Author

lanwen commented Aug 17, 2015

@oleg-nenashev only workflow PR is active. I'll rebase it myself. I'll help with FB

GitHubCommitNotifier_SettingCommitStatus(repository.getHtmlUrl() + "/commit/" + sha1)
);
repository.createCommitStatus(
sha1, state, build.getAbsoluteUrl(), msg, build.getProject().getFullName());
Copy link
Member

Choose a reason for hiding this comment

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

on previous line of code had new line, here not...

@KostyaSha
Copy link
Member

ok with 120, though it looks ugly for loggers and some variable creations :(

KostyaSha added a commit that referenced this pull request Aug 17, 2015
checkstyle with contributing guides
@KostyaSha KostyaSha merged commit 5b018be into jenkinsci:master Aug 17, 2015
@lanwen lanwen deleted the checkstyle branch August 17, 2015 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants