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

Bundle snippets #993

Merged
merged 5 commits into from
May 9, 2024
Merged

Bundle snippets #993

merged 5 commits into from
May 9, 2024

Conversation

confused-Techie
Copy link
Member

Like many PR's that have come before it, this PR bundles snippets right into the main repo.

This PR was kept at bay due to some open PRs on the snippets repository, but as discussed in #972 those PR's have now been merged, and we are good to go ahead and bundle snippets.

Once bundled, I'll update #512 as well as bring the two existing issues on the snippets repository over to this repo, then of course archive the original snippets repository.

The only changes that have occurred are present within the second commit, the first commit simply copied the contents of the snippets repository over.

@confused-Techie
Copy link
Member Author

For some reason the Editor Tests here are failing. The only big difference I can tell for these runs as compared to previously successful runs is a difference in NodeJS versions.

This run used v16.20.1 meanwhile the last successful run used v16.20.2 while a very small difference, it's the only thing obvious I can find. As for why this run went back a version, it looks like NodeJS was removed from the base image, since previously it was always found in the base image's cache. So without it being found I suppose it automatically downloaded it, and for whatever reason didn't grab the most up to date version. I'll test out specifying an exact version of NodeJS

@savetheclocktower
Copy link
Contributor

For some reason the Editor Tests here are failing. The only big difference I can tell for these runs as compared to previously successful runs is a difference in NodeJS versions.

This run used v16.20.1 meanwhile the last successful run used v16.20.2 while a very small difference, it's the only thing obvious I can find. As for why this run went back a version, it looks like NodeJS was removed from the base image, since previously it was always found in the base image's cache. So without it being found I suppose it automatically downloaded it, and for whatever reason didn't grab the most up to date version. I'll test out specifying an exact version of NodeJS

Now that @DeeDeeG has fixed the CI issues, a rebase from master should get this PR working, as it did on both of my open PRs.

@confused-Techie
Copy link
Member Author

Alright this one is ready for review, like @savetheclocktower pointed out with the CI bump it's almost totally passing. The only thing failing here is Codacy, which I think we have all decided doesn't matter in terms of merging a PR. (And in general should have a limit to avoid ever failing on a PR, but with so much code makes sense now is when it happens.)

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Yeah, Codacy has some fair points about defined but unused vars/functions, but nothing too drastic I don't think.

I am mostly trusting that this bundling was done as intended, actual specs appear to be green/passing. 👍

EDIT to add: Seeing the markdown file about the status of core-but-unbundled packages being updated is a good thing, too.

So, "rubber stamp / specs are passing approve" from me!

If we're done with the PEGjs files, some manual cleanup of the generated JS might be worthwhile, if anyone gets the itch to do some code cleanup. Codacy may well be a decent reference for this purpose. Not a blocker IMO.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Tiny markdown fixes?

packages/README.md Outdated Show resolved Hide resolved
packages/README.md Outdated Show resolved Hide resolved
@savetheclocktower
Copy link
Contributor

Most of the Codacy suggestions have to do with it not realizing that the spec files run in a spec environment. That one might be as simple as an .eslintrc.js in the spec directory matching the ones we already have in the repo for other packages' spec directories.

confused-Techie and others added 2 commits May 7, 2024 07:40
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks for catching the now invalid Markdown, I've merged your fixes.

But @savetheclocktower it looks like you're right about the specs being a major cause of Codacy being unhappy, so glad to see it shouldn't be too much of a pain to fix.

Otherwise once CI is passing if DeeDee's approval still applies we can go ahead and merge this one.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

👍

@confused-Techie confused-Techie merged commit 6c1f1ad into master May 9, 2024
102 checks passed
@confused-Techie confused-Techie deleted the bundle-snippets branch May 9, 2024 00:05
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.

3 participants