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

feat(onlychanged): allow to lint only changed files in the pull request #4

Closed
wants to merge 1 commit into from

Conversation

Mordred
Copy link

@Mordred Mordred commented Oct 1, 2019

Hi, I wanted to lint only files which has been changed in the Pull Request, so I added only-changed option.

Copy link
Owner

@mooyoul mooyoul left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! It looks cool additional feature 👍 but unfortunately, I need some changes before merging. Please check review details 🙇

@@ -0,0 +1 @@
node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, Good Catch 👍

@@ -12,6 +12,8 @@ inputs:
default: 'tslint.json'
pattern:
description: 'Glob pattern to match'
only-changed:
description: 'Apply glob patter only to changed files'
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice if we add restriction about this option - it only works with pull_event trigger

Copy link
Author

Choose a reason for hiding this comment

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

What you suggest?

@@ -41,7 +42,7 @@ const SeverityAnnotationLevelMap = new Map<RuleSeverity, "warning" | "failure">(
owner: ctx.repo.owner,
repo: ctx.repo.repo,
name: CHECK_NAME,
head_sha: ctx.sha,
head_sha: ctx.payload.pull_request ? ctx.payload.pull_request.head.sha : ctx.sha,
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you reference head.sha?

Copy link
Author

Choose a reason for hiding this comment

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

Because if you use ctx.sha - it won't appear in the PR: actions/toolkit#133 (comment)

@@ -71,7 +72,27 @@ const SeverityAnnotationLevelMap = new Map<RuleSeverity, "warning" | "failure">(
} else {
const linter = new Linter(options);

const files = glob.sync(pattern!);
let files = glob.sync(pattern!);
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice if this block can be cleaned.

e.g.

const files = await (async () => {
  const matched = glob.sync(pattern!);
  if (!onlyChanged) { 
    return matched;
  }
  
  const changedFiles = ctx.action === "pull_request" ? 
    (await octokit.pulls.listFiles(options)).data :
    await getChangedFilesSomeHow();

  return matched.filter((f) => changedFiles.includes(f));
})();

Copy link
Author

Choose a reason for hiding this comment

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

Why is this better? We are already in async function.

const changedFiles = response.data.map((f) => f.filename);
files = files.filter((f: string) => changedFiles.includes(f));
} else {
const response = await octokit.repos.getCommit({
Copy link
Owner

Choose a reason for hiding this comment

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

It won't get changed files correctly if user pushed multiple commits

Copy link
Author

Choose a reason for hiding this comment

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

All you have is ctx.sha so this should work OK.

@@ -122,6 +145,10 @@ const SeverityAnnotationLevelMap = new Map<RuleSeverity, "warning" | "failure">(
annotations,
},
});

if (result.errorCount) {
core.setFailed(`${result.errorCount} error(s), ${result.warningCount} warning(s) found`);
Copy link
Owner

Choose a reason for hiding this comment

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

Good Catch tho 👍

@@ -19,6 +19,7 @@ const SeverityAnnotationLevelMap = new Map([
const projectFileName = core.getInput("project");
const pattern = core.getInput("pattern");
const ghToken = core.getInput("token");
const onlyChanged = core.getInput("only-changed") || false;
Copy link
Owner

Choose a reason for hiding this comment

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

This file will be automatically updated. You don't have to change this file

Copy link
Author

Choose a reason for hiding this comment

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

I know - but without compiled file - I cannot test this branch in my project

@mooyoul mooyoul added enhancement New feature or request good first issue Good for newcomers labels Oct 1, 2019
@mooyoul mooyoul self-assigned this Oct 1, 2019
@aaomidi
Copy link

aaomidi commented Oct 18, 2019

Any updates on this?

@mooyoul
Copy link
Owner

mooyoul commented Oct 18, 2019

Hello @aaomidi, As you can see, I left a review of this PR and I will merge this if author updates PR with requested changes.

@aaomidi
Copy link

aaomidi commented Oct 18, 2019

Alright, I might make a PR that fixes this - would that be okay?

@mooyoul
Copy link
Owner

mooyoul commented Oct 18, 2019

Sounds great! It would be nice if you make a new PR 👍

@maxhawkins
Copy link

@aaomidi Did you make any progress on the new PR? I'm excited to see this feature land.

@aaomidi
Copy link

aaomidi commented Nov 6, 2019

Right now I've made my own repo building up on this. I eventually plan to PR back here, just haven't had the time :/

@mooyoul would you mind if the PR changed this from JS to TS too?

@mooyoul
Copy link
Owner

mooyoul commented Nov 6, 2019

Hello there, I’m looking into this. Stay tuned!

@Mordred Mordred closed this Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants