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

Refactor #6

Merged
merged 41 commits into from
Nov 17, 2022
Merged

Refactor #6

merged 41 commits into from
Nov 17, 2022

Conversation

edmundmiller
Copy link
Collaborator

@edmundmiller edmundmiller commented Nov 14, 2022

Adds some things, IDK if they're worthy of the CHANGELOG because it doesn't actually effect the end user.

  • Setup testing
  • Update license author docs
  • Updated the scripts
  • Setup linting/formatting
  • Updated the CI
  • Refactored the code (but no changes in functionality)

I just tried to keep things 1 to 1 identical with what was in the previous version and make it easier for others to work on the code base and pick it up when needed.

npm run build works now without global install
I don't know why I do this to myself. But it's done now.
https://github.com/OneSignal/OneSignal-Website-SDK
Yoinked the configs from here
I just want these for the typechecks
Doesn't work yet though
@MillironX
Copy link
Member

Nice! Thanks. This action has needed some love for a while but has been low on my priority list. It will take me a couple days before I can get through this, but it looks good on initial pass.

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

I have no idea what most of this is doing, but gosh darn it looks impressive to me 🚀

Copy link

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

It seems like the publish workflow should only run when tests pass? You can make one depend on the other so they don't run in parallel. Or maybe I'm misinterpreting your intentions.

I'm also wondering, does your code need to be async?

Since you're using TypeScript, I would define an interface for the parts of the release and asset objects that you intend to use. Or maybe there is a defined type for them already and you just need to use it?

@@ -17,7 +17,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: MillironX/setup-nextflow@v1
- uses: nf-core/setup-nextflow@v1.2.0

Choose a reason for hiding this comment

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

Seems like the nextflow-io org would actually be the ideal home for this action?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, not my department though! @ewels

Copy link
Member

@MillironX MillironX left a comment

Choose a reason for hiding this comment

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

Very nice! My only concern is that the tests look like they are hard-coded to look for a specific version of Nextflow to come from Octokit. I think it will be worth skipping those tests for now, and adding a mock later.

test('lastest_stable_release_data', async t => {
const result = await functions.latest_stable_release_data(t.context.octokit)
t.is(typeof result, 'object')
t.is(result.tag_name, 'v22.10.2')
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to throw errors when a new stable Nextflow version is released?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I meant to turn these into a mock, I just didn't know what I was even mocking at the beginning 😆 I'll get that sorted.

It's definitely going to throw an error when a new version gets released. I think a note about it is fine.

@MillironX MillironX merged commit 0aeab8d into master Nov 17, 2022
@Midnighter Midnighter deleted the refactor branch November 18, 2022 00:17
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.

4 participants