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

Add Java linting using google-java-format #30444

Closed
wants to merge 46 commits into from
Closed
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
0f2a3cf
Add java linting using google-java-format
nganbread Nov 20, 2020
6d2502b
Reverted temporary changes
nganbread Nov 20, 2020
437d396
Temporary lint violations for testing
nganbread Nov 20, 2020
814240e
Temporary lint violations for testing
nganbread Nov 20, 2020
56e4965
Revert package change
nganbread Nov 20, 2020
ff1e350
Temporary lint violations for testing
nganbread Nov 20, 2020
7bbaed8
Upgrade shelljs
nganbread Nov 20, 2020
1dc5865
Cleaned up lint-java
nganbread Nov 20, 2020
ecf4f05
Removed bad logs
nganbread Nov 20, 2020
04234da
Logging messages
nganbread Nov 20, 2020
f511cb3
Status code comments
nganbread Nov 20, 2020
7805e9b
Switching analysis executor to reactnativeandroid (for java)
nganbread Nov 20, 2020
014c912
Updated cache checkout_type after changing executor
nganbread Nov 20, 2020
af72091
Downgraded google-java-format to work with the container's version of…
nganbread Nov 20, 2020
3239b46
Downgrade shelljs
nganbread Nov 20, 2020
1c894ac
Remove sudo
nganbread Nov 20, 2020
01efa98
Merge branch 'master' into java-lint
nganbread Nov 26, 2020
ea1c626
Cleaned up args
nganbread Nov 27, 2020
288c334
Added replace arg
nganbread Nov 27, 2020
3a7231c
Merge branch 'java-lint' of https://github.com/nganbread/react-native…
nganbread Nov 27, 2020
daf291e
Prevent HTTP302 infinite loop
nganbread Nov 27, 2020
72ac710
Merge branch 'master' into java-lint
nganbread Nov 27, 2020
3faad5e
Updated API key
nganbread Nov 30, 2020
20459bf
Whitespace
nganbread Nov 30, 2020
bd718d0
Merge branch 'master' into java-lint
nganbread Nov 30, 2020
0b59067
Revert octokit changes
nganbread Nov 30, 2020
6c3ea16
Merge branch 'java-lint' of https://github.com/nganbread/react-native…
nganbread Nov 30, 2020
17c6771
Whitespace
nganbread Nov 30, 2020
8ce68d6
Fixed usage of new octokit API
nganbread Dec 1, 2020
ae62ea8
Merge branch 'master' into java-lint
nganbread Dec 1, 2020
1e03ff0
Removed test lint
nganbread Dec 1, 2020
8b98f2d
Merge branch 'java-lint' of https://github.com/nganbread/react-native…
nganbread Dec 1, 2020
e9f4877
Temporarily disable cache
nganbread Dec 1, 2020
610d6ce
Restore cache
nganbread Dec 1, 2020
d87b812
Fix step dependencies
nganbread Dec 1, 2020
3c348c3
Maybe?
nganbread Dec 1, 2020
13b8d92
Missing name
nganbread Dec 1, 2020
d666bbe
js_coverage now uses android job
nganbread Dec 1, 2020
1c37d70
Specific setup steps
nganbread Dec 1, 2020
940ddac
Missing colons
nganbread Dec 1, 2020
1e96851
Whitespace issue
nganbread Dec 1, 2020
386d6b3
Comment formatting
nganbread Dec 1, 2020
2f862c7
Reverted test lint violations, fixed github comments, fixed js lint i…
nganbread Dec 1, 2020
03ed1b1
Added pullbot tokens to build
nganbread Dec 1, 2020
640dfcd
Cleanup and formatting
nganbread Dec 1, 2020
cadbd11
Ran prettier. Removed shebang
nganbread Dec 2, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 29 additions & 15 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ defaults: &defaults
environment:
- GIT_COMMIT_DESC: git log --format=oneline -n 1 $CIRCLE_SHA1
# The public github tokens are publicly visible by design
- PUBLIC_PULLBOT_GITHUB_TOKEN_A: "a6edf8e8d40ce4e8b11a"
- PUBLIC_PULLBOT_GITHUB_TOKEN_B: "150e1341f4dd9c944d2a"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: &github_token_a "78a72af35445ca3f8180"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: &github_token_b "b1a98e0bbd56ff1ccba1"
- PUBLIC_PULLBOT_GITHUB_TOKEN_A: &github_pullbot_token_a "a6edf8e8d40ce4e8b11a"
- PUBLIC_PULLBOT_GITHUB_TOKEN_B: &github_pullbot_token_b "150e1341f4dd9c944d2a"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: &github_analysisbot_token_a "312d354b5c36f082cfe9"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: &github_analysisbot_token_b "07973d757026bdd9f196"

# -------------------------
# EXECUTORS
Expand All @@ -44,8 +44,10 @@ executors:
- GRADLE_OPTS: '-Dorg.gradle.daemon=false -Dorg.gradle.jvmargs="-XX:+HeapDumpOnOutOfMemoryError"'
- BUILD_THREADS: 2
# Repeated here, as the environment key in this executor will overwrite the one in defaults
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: *github_token_a
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: *github_token_b
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: *github_analysisbot_token_a
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: *github_analysisbot_token_b
- PUBLIC_PULLBOT_GITHUB_TOKEN_A: *github_pullbot_token_a
- PUBLIC_PULLBOT_GITHUB_TOKEN_B: *github_pullbot_token_b
reactnativeios:
<<: *defaults
macos:
Expand Down Expand Up @@ -236,17 +238,17 @@ jobs:
# Issues will be posted to the PR itself via GitHub bots.
# This workflow should only fail if the bots fail to run.
analyze_pr:
executor: nodelts
executor: reactnativeandroid
steps:
- restore_cache_checkout:
checkout_type: node
checkout_type: android
- run_yarn

- install_github_bot_deps

- run:
name: Install additional GitHub bot dependencies
command: sudo apt update && sudo apt install -y shellcheck jq
command: apt update && apt install -y shellcheck jq

- run:
name: Run linters against modified files (analysis-bot)
Expand All @@ -264,10 +266,10 @@ jobs:
# JOBS: Analyze Code
# -------------------------
analyze_code:
executor: nodelts
executor: reactnativeandroid
steps:
- restore_cache_checkout:
checkout_type: node
checkout_type: android
- setup_artifacts
- run_yarn

Expand All @@ -276,6 +278,11 @@ jobs:
command: scripts/circleci/exec_swallow_error.sh yarn lint --format junit -o ./reports/junit/eslint/results.xml
when: always

- run:
name: Lint Java
command: scripts/circleci/exec_swallow_error.sh yarn lint-java --check
when: always

- run:
name: Check for errors in code using Flow (iOS)
command: yarn flow-check-ios
Expand Down Expand Up @@ -850,27 +857,34 @@ workflows:

analysis:
jobs:
- setup
- setup:
name: setup_js

- setup:
name: setup_android
checkout_type: android
executor: reactnativeandroid

# Run lints on every commit other than those to the gh-pages branch
- analyze_code:
requires:
- setup
- setup_android
filters:
branches:
ignore: gh-pages

# Run code checks on PRs from forks
- analyze_pr:
requires:
- setup
- setup_android
filters:
branches:
only: /^pull\/.*$/

# Gather coverage
- js_coverage:
requires:
- setup
- setup_js
nightly:
triggers:
- schedule:
Expand Down
2 changes: 1 addition & 1 deletion bots/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ DANGER_GITHUB_API_TOKEN=[ENV_ABOVE] yarn danger pr https://github.com/facebook/r
The code analysis bot provides lint and other results as inline reviews on GitHub. It runs as part of the Circle CI analysis workflow.

If you want to test changes to the Code Analysis Bot, I'd recommend checking out an existing PR and then running the `analyze pr` command.
You'll need a GitHub token. You can re-use this one: `78a72af35445ca3f8180` `b1a98e0bbd56ff1ccba1` (just remove the space).
You'll need a GitHub token. You can re-use this one: `312d354b5c36f082cfe9` `07973d757026bdd9f196` (just remove the space).
So, for example:

```
Expand Down
150 changes: 82 additions & 68 deletions bots/code-analysis-bot.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ const converterSummary = {
'`flow` found some issues. Run `yarn flow check` to analyze your code and address any errors.',
shellcheck:
'`shellcheck` found some issues. Run `yarn shellcheck` to analyze shell scripts.',
'google-java-format':
'`google-java-format` found some issues. See https://github.com/google/google-java-format',
};

/**
Expand All @@ -57,6 +59,24 @@ const converters = {
}
},

'google-java-format': function(output, input) {
if (!input) {
return;
}

input.forEach(function(change) {
push(output, change.file, {
message: `\`google-java-format\` suggested changes:
\`\`\`diff
${change.description}
\`\`\`
`,
line: change.line,
converter: 'google-java-format',
});
});
},

flow: function(output, input) {
if (!input || !input.errors) {
return;
Expand Down Expand Up @@ -113,30 +133,6 @@ const converters = {
},
};

function getShaFromPullRequest(octokit, owner, repo, number, callback) {
octokit.pullRequests.get({owner, repo, number}, (error, res) => {
if (error) {
console.error(error);
return;
}

callback(res.data.head.sha);
});
}

function getFilesFromPullRequest(octokit, owner, repo, number, callback) {
octokit.pullRequests.listFiles(
{owner, repo, number, per_page: 100},
(error, res) => {
if (error) {
console.error(error);
return;
}
callback(res.data);
},
);
}

/**
* Sadly we can't just give the line number to github, we have to give the
* line number relative to the patch file which is super annoying. This
Expand Down Expand Up @@ -166,7 +162,15 @@ function getLineMapFromPatch(patchString) {
return lineMap;
}

function sendReview(octokit, owner, repo, number, commit_id, body, comments) {
async function sendReview(
octokit,
owner,
repo,
pull_number,
nganbread marked this conversation as resolved.
Show resolved Hide resolved
nganbread marked this conversation as resolved.
Show resolved Hide resolved
commit_id,
body,
comments,
) {
if (process.env.GITHUB_TOKEN) {
if (comments.length === 0) {
// Do not leave an empty review.
Expand All @@ -181,19 +185,14 @@ function sendReview(octokit, owner, repo, number, commit_id, body, comments) {
const opts = {
owner,
repo,
number,
pull_number,
commit_id,
body,
event,
comments,
};

octokit.pullRequests.createReview(opts, function(error, res) {
if (error) {
console.error(error);
return;
}
});
await octokit.pulls.createReview(opts);
} else {
if (comments.length === 0) {
console.log('No issues found.');
Expand All @@ -216,7 +215,7 @@ function sendReview(octokit, owner, repo, number, commit_id, body, comments) {
}
}

function main(messages, owner, repo, number) {
async function main(messages, owner, repo, pull_number) {
// No message, we don't need to do anything :)
if (Object.keys(messages).length === 0) {
return;
Expand All @@ -234,40 +233,54 @@ function main(messages, owner, repo, number) {
auth: process.env.GITHUB_TOKEN,
});

getShaFromPullRequest(octokit, owner, repo, number, sha => {
getFilesFromPullRequest(octokit, owner, repo, number, files => {
let comments = [];
let convertersUsed = [];
files
.filter(file => messages[file.filename])
.forEach(file => {
// github api sometimes does not return a patch on large commits
if (!file.patch) {
return;
}
const lineMap = getLineMapFromPatch(file.patch);
messages[file.filename].forEach(message => {
if (lineMap[message.line]) {
const comment = {
path: file.filename,
position: lineMap[message.line],
body: message.message,
};
convertersUsed.push(message.converter);
comments.push(comment);
}
}); // forEach
}); // filter

let body = '**Code analysis results:**\n\n';
const uniqueconvertersUsed = [...new Set(convertersUsed)];
uniqueconvertersUsed.forEach(converter => {
body += '* ' + converterSummary[converter] + '\n';
});
const opts = {
owner,
repo,
pull_number,
};

sendReview(octokit, owner, repo, number, sha, body, comments);
}); // getFilesFromPullRequest
}); // getShaFromPullRequest
const {data: pull} = await octokit.pulls.get(opts);
const {data: files} = await octokit.pulls.listFiles(opts);

const comments = [];
const convertersUsed = [];

files
.filter(file => messages[file.filename])
.forEach(file => {
// github api sometimes does not return a patch on large commits
if (!file.patch) {
return;
}
const lineMap = getLineMapFromPatch(file.patch);
messages[file.filename].forEach(message => {
if (lineMap[message.line]) {
const comment = {
path: file.filename,
position: lineMap[message.line],
body: message.message,
};
convertersUsed.push(message.converter);
comments.push(comment);
}
}); // forEach
}); // filter

let body = '**Code analysis results:**\n\n';
const uniqueconvertersUsed = [...new Set(convertersUsed)];
uniqueconvertersUsed.forEach(converter => {
body += '* ' + converterSummary[converter] + '\n';
});

await sendReview(
octokit,
nganbread marked this conversation as resolved.
Show resolved Hide resolved
nganbread marked this conversation as resolved.
Show resolved Hide resolved
owner,
repo,
pull_number,
pull.head.sha,
body,
comments,
);
}

let content = '';
Expand Down Expand Up @@ -331,6 +344,7 @@ process.stdin.on('end', function() {

const number = process.env.GITHUB_PR_NUMBER;

// intentional lint warning to make sure that the bot is working :)
main(messages, owner, repo, number);
(async () => {
await main(messages, owner, repo, number);
})();
});
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"flow-check-android": "flow check --flowconfig-name .flowconfig.android",
"lint": "eslint .",
"lint-ci": "./scripts/circleci/analyze_code.sh && yarn shellcheck",
"lint-java": "node ./scripts/lint-java.js",
"shellcheck": "./scripts/circleci/analyze_scripts.sh",
"clang-format": "clang-format -i --glob=*/**/*.{h,cpp,m,mm}",
"format": "npm run prettier && npm run clang-format",
Expand Down
2 changes: 1 addition & 1 deletion scripts/circleci/analyze_code.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ GITHUB_REPO=${CIRCLE_PROJECT_REPONAME:-react-native}
export GITHUB_OWNER
export GITHUB_REPO

cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run flow-check-ios --silent --json; echo flow; npm run flow-check-android --silent --json) | GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" node bots/code-analysis-bot.js
cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run flow-check-ios --silent --json; echo flow; npm run flow-check-android --silent --json; echo google-java-format; node scripts/lint-java.js --diff) | GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" node bots/code-analysis-bot.js

STATUS=$?
if [ $STATUS == 0 ]; then
Expand Down
Loading