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

TypeScript rewrite, new options, improved workflows & whatnot #30

Open
wants to merge 32 commits into
base: typescript-rewrite
Choose a base branch
from

Conversation

EndBug
Copy link

@EndBug EndBug commented Sep 14, 2020

Disclaimer: I know this is a huge PR and it's not ideal to have so many changes in a single one. The only reason it's this way it's because I needed to develop it for my README and I couldn't wait for all the changes to be gradually merged in this repo.

Changelog:

  • The action is now written in TypeScript, providing static type checking.
  • There are now 3 new options:
    • LINES: you can use different numbers of lines.
    • NO_COMMIT: the action won't commit the resulting file.
    • NO_DEPENDABOT: the action will filter out events triggered by merging or closing a PR by Dependabot. The reason for this is that I found my actions to be mainly populated by this kind of events since I merge a lot of these PRs.
  • The action can now fetch more events, if after the first 100 don't contain enough eligible ones: it will keep requesting events until they end.
  • The dist folder is updated automatically by a workflow (update.yml), which also runs prettier.
  • The test workflow has been removed, since the update workflow is already handling the testing (it doesn't run on a matrix, but I think that's fine: almost nobody will need to run this action on macOS, and the action doesn't have platform-specific code)

Copy link
Owner

@jamesgeorge007 jamesgeorge007 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 the PR @EndBug, left a couple of comments.

description: The number of lines to generate
default: '5'
required: false
NO_COMMIT:
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain the motivation behind introducing this input param?

Copy link
Author

Choose a reason for hiding this comment

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

Which one? NO_COMMIT or LINES?

  • NO_COMMIT: if someone wants to do some other edit to the README they can do it and then push all the changes in a single commit.
  • LINES: they can select the number of lines from the workflow

Copy link
Owner

Choose a reason for hiding this comment

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

We're checking to see if there is any difference between the content enclosed within the respective comments and the latest data being pulled. Any change made to other sections of the README doesn't affect this.

Copy link
Author

Choose a reason for hiding this comment

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

@jamesgeorge007 That's not the issue: I may want to do some other changes to the README and then commit everything together, instead of having multiple commits.

.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/node.js.yml Outdated Show resolved Hide resolved
@jamesgeorge007 jamesgeorge007 added the feature New feature label Sep 19, 2020
@EndBug
Copy link
Author

EndBug commented Sep 23, 2020

Also, I was thinking of changing the emoji for issue comments to 💬 because it looks better (at least on Windows). Is that ok?

@jamesgeorge007
Copy link
Owner

Also, I was thinking of changing the emoji for issue comments 💬 because it looks better (at least on Windows). Is that ok?

Looks better on windows? I didn't get your point.

Copy link
Owner

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts.

@EndBug
Copy link
Author

EndBug commented Sep 26, 2020

Also, I was thinking of changing the emoji for issue comments 💬 because it looks better (at least on Windows). Is that ok?

Looks better on windows? I didn't get your point.

In my opinion, that emoji looks better if compared to the current one (🗣) which, at least on my machine (that runs Windows), looks like this:
image

I just think the other emoji is clearer and fits better with the other ones. That's just my personal opinion: what do you think about it?

.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Owner

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

Thank you so much for your patience. Please resolve the conflicts.

@EndBug
Copy link
Author

EndBug commented Nov 21, 2020

Ok, now there should be no more conflicts

@EndBug
Copy link
Author

EndBug commented Nov 28, 2020

Just as a sidenote:
I understand that this is a huge PR and everything, but I'd like you to make a decision on whether to merge this or not.
It's already the second time I resolve conflicts because you keep updating the master branch without merging the PR. I'm fine with you not merging the PR, but I'd like to know whether you're eventually going to do it or not. If you want to keep waiting, just be aware that you'll have to resolve the merge conflicts yourself (you already have push access to this PR's branch)

@jamesgeorge007
Copy link
Owner

I'm sorry for making you wait; it's just that there is plenty on my plate. I had to land a couple of PR's as the need came in, and yeah, they were small. Thank you so much for putting in the efforts here; I'll take a look as time permits.

@tuunit tuunit mentioned this pull request May 20, 2023
@tuunit tuunit changed the base branch from master to typescript-rewrite May 31, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants