Skip to content

Commit

Permalink
Add Java linting using google-java-format (#30444)
Browse files Browse the repository at this point in the history
Summary:
Adds `google-java-format` linting for all `.java` files in the `ReactAndroid/` folder

- Linting requires java and is now performed on the android container
- https://github.com/google/google-java-format

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Internal] [Added] - Linting for *.java files (google-java-format)

Pull Request resolved: #30444

Test Plan: See this example PR for lint comments: #30512

Reviewed By: hramos

Differential Revision: D25253627

Pulled By: nganbread

fbshipit-source-id: e39e4411bf09a96c054afaf6c12b3d05a80f40fa
  • Loading branch information
nganbread authored and facebook-github-bot committed Dec 7, 2020
1 parent 503a6f4 commit defdb6d
Show file tree
Hide file tree
Showing 6 changed files with 319 additions and 85 deletions.
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 @@ -234,17 +236,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 @@ -262,10 +264,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 @@ -274,6 +276,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 @@ -847,27 +854,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,
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,
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 @@ -60,6 +60,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

0 comments on commit defdb6d

Please sign in to comment.