-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add context awareness #4
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
Conversation
On this one considering it seems little of the changes are our own, and mostly changes in Although we now have tests in the main branch, is it possible or easy to bring the GitHub actions testing over here so we can just verify everything is happy there? Or if need be I can always get this local and test that way. Would just be nice for that peace of mind before approving |
Yep, merged @mauricioszabo, what was the logic behind vendoring |
Seems unfortunate that the test script is no longer valid with the changes made to this repo. Lemme try just testing this locally and seeing what we need to do to change the test script to work here, and of course check that tests are running properly |
Was a quick fix. CI is passing now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not the original author or too familiar here, based on my reasons above I'm gonna give an approval here. Little of the changes seem to be ours, and our tests are passing just fine with minimal changes. Lets advance the march towards upgraded Electron!
@savetheclocktower submodules weren't working correctly with |
"url": "https://github.com/atom/git-utils.git" | ||
"url": "https://github.com/git-utils/git-utils.git" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/atom/git-utils/issues" | ||
"url": "https://github.com/git-utils/git-utils/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but I suppose this was a typo.
(Repo git-utils/git-utils
should be --> pulsar-edit/git-utils
?)
The former resulting in a 404 not found, and the latter being this repo (our fork of Atom's package, the repo the present PR and files exist at now) and thus accurate/apropos.
(Pardon the lateness of this comment, I was away for a bit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, good catch. I'll fix it tomorrow.
None of these are my commits, but we should land this work to
master
as part of the march toward PulsarNext.Context-awareness is needed so that we can upgrade Electron. The
updated-latest-electron
branch of Pulsar has been pointing itsgit-utils
dependency to this branch. We should land it, set up this repo to publish to NPM, then publish it as@pulsar-edit/git-utils
version6.0.0
. (Current stable Pulsar depends on the currentgit-utils
present in NPM, so we don’t need to publish the current version first.)While I was migrating some of the other modules, I took the opportunity to migrate this module to N-API. I got the tests passing, so everything seems sound — but that’s as far as I took it. Since this branch is what PulsarNext has been using for months, I think we should start with this, and then we can consider the N-API migration at some point in the future.