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

Bump snippets to bb00f9 #408

Merged
merged 4 commits into from
Mar 4, 2023

Conversation

savetheclocktower
Copy link
Contributor

Bumps snippets from fe00fd6933fa33f819d14cdd6938d538d25ba1dd to bb00f909c6c645b173f27346875d8fa0c7af09f7.

Provides the following PRs:

Let me know if this should be done a different way. I ran

npm install "https://github.com/pulsar-edit/snippets.git#bb00f909c6c645b173f27346875d8fa0c7af09f7"

and npm converted it to the syntax you see in the package.json. I’m also not wild about how much yarn.lock got changed for such a small update.

@Spiker985
Copy link
Member

Something I've noticed, is that doing any npm operations, updates the urls of the yarn.lock to no longer point to yarnpkg

So I've deleted the yarn.lock, and recreated it with a simple yarn install so it's maintained

We do use yarn for this repo, but both so long as either npm or yarn update the dependencies appropriately - they should both be fine. I do recommend having yarn recreate that lock file though, please

@savetheclocktower
Copy link
Contributor Author

savetheclocktower commented Feb 27, 2023

Ah, muscle memory always leads me to npm. I should setup a dotenv in the directory to yell at me if I try to upgrade via npm instead of yarn.

@savetheclocktower
Copy link
Contributor Author

Oof. yarn install works until:

error An unexpected error occurred: "ENOENT: no such file or directory, scandir '/Users/andrew/Code/JavaScript/pulsar/node_modules/styleguide/node_modules/atom-select-list/node_modules'".

/Users/andrew/Code/JavaScript/pulsar/node_modules/styleguide exists but is empty.

@Spiker985
Copy link
Member

I'd drop your entire node_modules folder, drop the yarn.lock file, and then redo the yarn install

Since that's going to replicate how our CI environments will start anyway

@savetheclocktower
Copy link
Contributor Author

OK, yarn.lock's diff is nowhere near as big now (though still somewhat big).

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 2, 2023

Just a comment on the yarn.lock situation, I have found that this is a thing going back at least as far as npm 8.x.

To a certain extent, this rises to the level of being a bug (npm/cli#5126).

(I think they change it intentionally, but it'd be nice if they didn't change it at all, IMO, which was the status quo in npm 6.x that they have since changed in newer npm.)

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Thanks a ton for doing this, and thanks a ton for properly including the PR's that have been merged!

@DeeDeeG
Copy link
Member

DeeDeeG commented Mar 2, 2023

So, unfortunately, newer npm doesn't really play nice with yarn.lock, and I'd say it's preferable to update yarn.lock only with Yarn.

(And to clarify, that is yarn v1 anywhere in the Pulsar org, also known as yarn on the npm package registry (not yarn 2.x or 3.x known as "Yarn Berry"), so you can get it with npm install -g yarn, or in recent NodeJS with corepack, you can do corepack enable).

If you want to do just the package.json change, that'd be okay, but syncing the lockfile in the same PR when dependencies in package.json change is definitely better, IMO.

DeeDeeG

This comment was marked as resolved.

@savetheclocktower
Copy link
Contributor Author

So I specifically did re-do this with yarn; that was the “Fix yarn.lock” commit. But please do give it a shot and let me know if you were able to get the diff smaller.

@DeeDeeG

This comment was marked as resolved.

@savetheclocktower
Copy link
Contributor Author

Let me know if you want me to push this to this PR's branch (assuming we are granted write access to the PR branch.)

By all means! You've got write access to this branch. Appreciate it.

@DeeDeeG DeeDeeG dismissed their stale review March 2, 2023 05:23

yarn.lock diff was minimized, so the "requested change" from this review comment has been implemented

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.

Approving that this is a validly performed dependency bump.

I can also confirm this PR is bumping to the commit at the tip of the "snippets" repo's master branch at the moment I'm writing this (bb00f909c6), which I confirm does include at least the four PRs mentioned in this PR's description.

(Note: Some PRs were merged at "snippets" repo with merge commit messages that don't mention the PR # in the first line, but they do mention the PR number in the body text/subsequent lines of the merge commit message.)

git log --graph --oneline output (click to expand):

It's easier to see in graph view, but this output is much more readable if you run this locally, due to color-coded graph lines.

% git log --oneline --graph 388d9b6~3...origin/master
*   bb00f90 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #5 from Sertonix/cleanup-and-master
|\  
| *   0bbbeac Merge branch 'master' into cleanup-and-master
| |\  
| |/  
|/|   
* |   12eec6d Merge pull request #6 from pulsar-edit/fix-tests
|\ \  
| * \   3cd111d (origin/fix-tests) Merge branch 'master' into fix-tests
| |\ \  
| * | | fe00fd6 Fix failing specs
* | | |   76a32e5 Fix global flag on snippet transformations
|\ \ \ \  
| |_|/ /  
|/| | |   
| * | | c5c73c0 Update version to 1.7.0
| * | | cebceda Remove implicit `g` flag from snippet transformations
|/ / /  
* | |   d0c574a [feat] Add command property to Snippet entries
|\ \ \  
| |/ /  
|/| |   
| * | 91076f5 Update .github/workflows/pulsar_test.yml
| * | 8f87140 Apply suggestions from code review
| * | 6410899 Fix spec failure on Windows
| * | 159459f Workflow attempt 2
| * | d4835d8 Testing a branch of action-pulsar-dependency
| * | 5bf4a85 Debounce the user-snippets-file-changed handler
| * | 34bd7ff Fix failing test
| * | e8c3495 Update README.md.
| * | 8133849 Add tests.
| * | d944225 Add ability to assign a snippet to a command name
| * | 29deddc Add eslint
|/ /  
* | 009b767 (deedeeg/master) Fix typo
| * 4b9d45c remove coffeelint
| * 26695d8 cleanup and rename
|/  
*   388d9b6 Merge pull request #2 from Sertonix/remove-fs-plus
[ . . . snipped more commits . . . ]

@confused-Techie
Copy link
Member

Alright, with two approvals on this PR I'll go ahead and merge it.

Thanks @savetheclocktower for this one, we appreciate you!

@confused-Techie confused-Techie merged commit c5d1acb into pulsar-edit:master Mar 4, 2023
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.

4 participants