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 autocomplete-atom-api #476

Merged
merged 10 commits into from
May 2, 2023
Merged

Bundle autocomplete-atom-api #476

merged 10 commits into from
May 2, 2023

Conversation

confused-Techie
Copy link
Member

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

In this PR I hope to finally got the majority of the last bundles we need for core packages.

But the PR is starting off with only 2 additional packages, since symbols-view has proven difficult in the past, so extra care is being taken while it is merged to ensure against regressions.

As more packages are available here, this will be updated.

EDIT:

This PR, at the request of @mauricioszabo will now only bundle a single package.

Which at this time is autocomplete-atom-api


For easy reference, for myself and others, here is the last core packages that need to be bundled:

  • snippets: Per savetheclocktower's request, this will not be bundled yet
  • autocomplete-atom-api: ✅
  • symbols-view: WIP - Determine the test weirdness occurring here first
  • find-and-replace: ❌ - Has open PRs cannot bundle
  • fuzzy-finder: ❌ - Has open PRs cannot bundle
  • github: ❌ - Has open PRs cannot bundle
  • notifications: Previously attempted on Additional Bundling of Core Packages #424 encountered increased failure rate
  • spell-check: Next in Line
  • tree-view: Has open PRs, but likely could be addressed prior to merge

@savetheclocktower
Copy link
Contributor

@mauricioszabo, would you rather that symbols-view be moved into core now, or should we wait until the refactor?

@confused-Techie, our plan is for symbols-view to be rewritten as a consumer of symbols and for the default ctags provider to be moved to a package like symbol-provider-ctags and serve as the default provider of symbols. I don't know what has been so difficult about migrating symbols-view in the past, but I imagine it could have something to do with the ctags package, which relies on native bindings.

In this scenario, we'd want both symbols-view and symbol-provider-ctags to still be bundled with the editor, whether or not they lived in the main repo. So if this will be a pain, I think we could easily just hold off for a few weeks or so until I can finish up the refactor.

From my perspective, I was planning to apply the refactor in place rather than start a new repo, so it doesn't matter to me whether that happens in a separate symbols-view repo or within the main pulsar repo.

@confused-Techie
Copy link
Member Author

I'd be totally open to waiting on a rewrite for symbols view.
As even now the tests are failing at a higher than expected rate during this migration. So ideally a rewrite could help to alleviate those issues. As you can see there are still a few other items that need to be bundled, which I could work on for this PR

@mauricioszabo
Copy link
Contributor

A suggestion: separate all bundling on a different PR, so we can fix tests for each of these packages when we bundle them :)

@confused-Techie
Copy link
Member Author

@mauricioszabo that's a fair point. Suppose I was wanting to reduce the amount of PRs required to finish our bundling.

But if we would rather have on bundle per PR (at least for these last few, that sounds fine by me and I'll modify this PR as such.

@confused-Techie confused-Techie changed the title Bundle autocomplete-atom-api & symbols-view Bundle autocomplete-atom-api May 2, 2023
@confused-Techie confused-Techie marked this pull request as ready for review May 2, 2023 02:01
@confused-Techie
Copy link
Member Author

This PR is now ready for review, just letting the tests run, but now this PR only bundles autocomplete-atom-api instead of multiple package. Ideally now each new bundle will be it's own isolated PR.

@confused-Techie
Copy link
Member Author

Looks like all tests here are passing, and hope nobody minds but gonna go ahead and merge this one.

As it leaves zero change in how Pulsar behaves, and I'm wanting to get a push on bundling tree-view so that by the time our survey comes back we already have it bundled so that we can make the changes we need.

@confused-Techie confused-Techie merged commit c8aee53 into master May 2, 2023
@confused-Techie confused-Techie deleted the hard-bundles branch May 2, 2023 03:33
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