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

paginate over all changed files and sync and add "sync-labels" option to enable removing labels #63

Closed

Conversation

kevinbuhmann
Copy link

@kevinbuhmann kevinbuhmann commented Apr 19, 2020

Build process

  • run fomatter
    • This might be problem in my environment, but it looks like the code on master is not formatted via the npm run format command. I see the same changes in other open PRs so I don't think this is isolated to just me.
  • generate dist/index.js in a deterministic manner
    • When I run npm run build it adds webpack-looking wrapper code to dist/index.js every time. It changes the js file even in main.ts is not changed. I changed the build script to ncc build ./src/main.ts to compile the TypeScript source directly and it seems to work better.
    • It is possible that this is a problem in my environment, but if it is, I can't seem to figure it out. Like the formatter issue, other open PRs seem to have unrelated changes in their dist/index.js as well so this also does not seem isolated to just me.

Fixes

Features

  • add "sync-labels" options to enable removing labels

When I run 'npm run build' it adds webpack code to dist/index.js
every time. It changes the js file even in main.ts is not changed.
@ericcornelissen
Copy link
Contributor

Nice work on the "sync-labels" feature 👍

Perhaps it is an idea to limit this PR to just that feature to increase the likelihood of it being merged? Pagination is already implemented in #43 and should probably be merged there. Doing the formatting in this PR makes it difficult to review the meaningful changes in this PR (and causes unnecessary merge conflicts).

@kevinbuhmann
Copy link
Author

I can rebase this later and see if the formatting is fixed on master. I only made those changes in this PR because I could not get a passing build otherwise.

@ericcornelissen
Copy link
Contributor

I can rebase this later and see if the formatting is fixed on master. I only made those changes in this PR because I could not get a passing build otherwise.

Makes sense. The formatting is not yet fixed on master, but it seems you will no longer get a failing build as a result of incorrect formatting.

@kevinbuhmann
Copy link
Author

The other issue is that npm run build with master checked out changes the dist/index.js build even though none the TypeScript is changed. I am unsure how to handle this.

@ericcornelissen
Copy link
Contributor

The other issue is that npm run build with master checked out changes the dist/index.js build even though none the TypeScript is changed. I am unsure how to handle this.

I can't help you with that, could be due to a variety of reasons. But I don't think it matters much. Merge conflicts in the dist/index.js file are straightforward to resolve by the maintainers of this action and don't affect reviewing the changes, so I wouldn't worry about it.

@dakale
Copy link
Contributor

dakale commented Sep 8, 2020

Thanks for this, and sorry for the huge delay. The maintainership of this action has changed a few times so some older items like this fell off the radar

I merged your changes via #96 to make it a little easier / faster as Im going through and cleaning up a bit

@dakale dakale closed this Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove labels for reverted changes
3 participants