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 markdown-preview, styleguide, wrap-guide #374

Merged
merged 20 commits into from
Feb 25, 2023
Merged

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Feb 8, 2023

This PR does nothing new or fancy.

Just continuing (yet again) the process of bundling and testing our core packages.

This PR bundles the following packages:

  • symbols-view (Note this PR no longer bundles symbols-view read below for details)
  • markdown-preview
  • styleguide
  • wrap-guide

Hopefully after this we will only need one last major bundling PR to grab the vast majority of the others.
Than from there we still will have a few others to bundle that have active and open PRs or issues.

@confused-Techie confused-Techie changed the title Even more bundles Bundle symbols-view, markdown-preview, styleguide, wrap-guide Feb 8, 2023
@Daeraxa
Copy link
Member

Daeraxa commented Feb 8, 2023

Just adding that the packages/README.md should probably up updated in this PR too to edit the repo link to the new relative link.

@confused-Techie
Copy link
Member Author

So a quick peek at how the tests are looking here:

  • find-and-replace: 53 Failures (51-ish expected)
  • tree-view: 2 Failures (2 Expected)
  • symbols-view: 25 Failures (2 Expected)

So while mostly everything looks fine, we are getting far more failures on symbols-view than we would expect.

I'll go ahead and look into this, but until then this shouldn't be merged.

@confused-Techie
Copy link
Member Author

So something interesting, I've confirmed the files I brought over for symbols-view have only received a single new commit from the tagged GitHub version we were using. And even getting the files of that specific commit I am still seeing many failures.

Almost makes me wonder if these aren't failing because of something else? As really the installing process shouldn't be any different now than it was with the previous symbols-view being downloaded from GitHub

@confused-Techie
Copy link
Member Author

Alright, I've tried to replicate the test results we have received otherwise to our previous expected fail rate, but can't seem to get it right.

After much testing locally I wasn't able to determine what the difference is between these repos when bundled or installed from GitHub.

Going as far as to check file diffs from node_modules once either was installed.

For the time being I've opted to instead remove symbols-view as a bundled package and hope to focus on it at a later time likely as a singular PR

@confused-Techie confused-Techie changed the title Bundle symbols-view, markdown-preview, styleguide, wrap-guide Bundle ~~symbols-view~~, markdown-preview, styleguide, wrap-guide Feb 15, 2023
@confused-Techie confused-Techie changed the title Bundle ~~symbols-view~~, markdown-preview, styleguide, wrap-guide Bundle ~symbols-view~, markdown-preview, styleguide, wrap-guide Feb 15, 2023
@confused-Techie confused-Techie changed the title Bundle ~symbols-view~, markdown-preview, styleguide, wrap-guide Bundle markdown-preview, styleguide, wrap-guide Feb 15, 2023
@confused-Techie
Copy link
Member Author

To add the test results now that everythings come back:

  • find-and-replace: 63 Failures - A tad high, but within expected range
  • symbols-view: 2 Failures - Expected 2
  • tree-view: 2 Failures - Expected 2

Additionally the intel mac build is failing in the same way seen on other PRs. Leads me to believe something has changed on cirrus' end rather than ours. But still noteworthy

@confused-Techie
Copy link
Member Author

@pulsar-edit/core if anyone's willing to take a look at this that'd be rad.

Otherwise as it's technically a non-code change we can go ahead and merge it.

All it does is move the code from a repo to our pulsar repo

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 24, 2023

@confused-Techie do you mind if this is rebased on latest master, or has master merged into it?

A bit paranoid, but IMO it'd still be nice to be reassured the intel Mac Cirrus job works fine with the "pin Python to 3.10" fix from #384.

EDIT: I volunteer to do the rebase (or merge master in here if you prefer) if it saves you some time.

@confused-Techie
Copy link
Member Author

@DeeDeeG you are totally free to do it, if you've got the workflow up.

Otherwise I'll also go ahead and try to do it sometime tonight.

Thanks for taking a look

@confused-Techie
Copy link
Member Author

Alright @DeeDeeG thanks for merging our changes the other day, but with #399 being merged, we had to do this yet again.

So I've done so and taking a look at the tests everything, almost looks perfect.

The package tests are all within expected range:

  • find-and-replace: 57
  • symbols-view: 2
  • tree-view: 2

Although the Cirrus CI intel_mac and windows tasks both failed. Although due to failing to notarize, and timing out respectively so not sure if that's the fault of this PR.

But I've gone ahead and requested a rerun, so hopefully we can finally review and merge this one, once those are done if things look good.

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.

CI looks good now!

Thanks for waiting on those runs/re-running, etc.

@DeeDeeG DeeDeeG merged commit 72ca237 into master Feb 25, 2023
@Spiker985 Spiker985 deleted the even-more-bundles branch February 25, 2023 10:51
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