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: static modules #8679

Merged
merged 11 commits into from
Mar 2, 2021
Merged

feat: static modules #8679

merged 11 commits into from
Mar 2, 2021

Conversation

viceice
Copy link
Member

@viceice viceice commented Feb 13, 2021

Changes:

Convert generated to static modules

Context:

#8647 #6747

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added unit tests, or
  • No new tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@viceice viceice marked this pull request as ready for review February 13, 2021 09:45
@JamieMagee
Copy link
Contributor

Is generate-imports required anymore?

@viceice
Copy link
Member Author

viceice commented Feb 13, 2021

Yes, for node schedule and pip_setup extractor. Maybe it should be renamed. 😏

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

We should add a test for each of these modules to make sure that all (relevant) subdirectories are imported and match the import name.

tools/generate-imports.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member Author

viceice commented Feb 13, 2021

Strange, not sure why it fails with oom. 🤔

Copy link
Contributor

@JamieMagee JamieMagee left a comment

Choose a reason for hiding this comment

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

Ugh, leaving a comment from the GitHub app starts a full review.

EDIT: I left this comment earlier but had to 'approve' it on web. I have no idea what's going on here.

tools/generate-imports.ts Outdated Show resolved Hide resolved
@rarkins
Copy link
Collaborator

rarkins commented Feb 13, 2021

Yes, happens to me often. It defaults to pending comments in a review, not sure if you can change that

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Needs 2 missing managers added :)

@rarkins rarkins marked this pull request as draft February 28, 2021 15:20
@viceice viceice marked this pull request as ready for review March 2, 2021 14:44
@viceice viceice requested a review from rarkins March 2, 2021 14:45
@viceice
Copy link
Member Author

viceice commented Mar 2, 2021

We should add a test for each of these modules to make sure that all (relevant) subdirectories are imported and match the import name.

expect(Array.from(dss.keys())).toEqual(Object.keys(loadedDs));

expect(Array.from(mgrs.keys())).toEqual(Object.keys(loadedMgr));

expect(Array.from(platforms.keys())).toEqual(Object.keys(loadedMgr));

expect(Array.from(vers.keys())).toEqual(Object.keys(loadedVers));

Already tested 🙃

@viceice viceice merged commit 91d2b4e into master Mar 2, 2021
@viceice viceice deleted the feat/static-modules branch March 2, 2021 15:57
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 24.71.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants