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 dugite && Bump package.json version #2

Merged
merged 3 commits into from Oct 23, 2023
Merged

Conversation

confused-Techie
Copy link
Member

This PR bumps the version of dugite installed with this package. And bumps the next version of the package itself to prep for a new version to be published.

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.

I assume this is motivated by a desire to dedupe different dugite versions in pulsar core repo?

Approving because tests pass. There's so very little code in this repo, I assume if tests are checking anything at all then they muct be exercising a code path that uses dugite. Like, I hardly think there can be more than one code path to hit in this entire repo, heh.

EDIT: Changing this review to Comment, not Approve, because in fact I don't think the tests exercise the getDiff() function that uses dugite, or the translateLines() function that calls getDiff() (so also indirectly uses dugite) from src/index.ts.

Tests only exercise two other functions, translateLinesGivenDiff() and diffPositionToFilePosition(), which do not appear to use dugite even indirectly, as far as I can tell.

So, some better testing would be needed to really confirm this still works with bumped dugite.

I leave it to you to merge this since I'm not 100% familiar with the motivation for this change, but it seems like a good idea. Newer dugite releases seem to be about modernizing the git implementation and some general light maintenance, so nothing too worrisome in those release notes: https://github.com/desktop/dugite/releases

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 21, 2023

Would be super mega amazing if we added more tests in this repo to exercise some of that dugite functionality.

Test(s) for getDiff() and/or translateLines() functions from src/index.ts, specifically.

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks a ton for giving this one a review. And you are 100% correct about the motivations for this change.

But thanks for investigating the tests here, and seems you are correct that these changes aren't being tested at all. I suppose prior to merging this one I'll see if I can work out some tests for this so that we can be confident things are working the way it's intended.

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 22, 2023

And if added tests are too much, we can find a way to use dugite here in some practical IRL/manual testing.

@confused-Techie
Copy link
Member Author

And if added tests are too much, we can find a way to use dugite here in some practical IRL/manual testing.

It seems reasonable enough to me.

I was honestly on the fence about how deep to dive into this package when making this change, but now finding out that it's missing some important elements, I'll take the time to fully bring it in line with how we do most things, such as removing the TypeScript nonsense

@confused-Techie confused-Techie mentioned this pull request Oct 23, 2023
@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 23, 2023

If I can be of assistance with the merge conflict, let me know, but lots of stuff getting done around this fork, kind of exciting, not gonna lie.

@confused-Techie
Copy link
Member Author

If I can be of assistance with the merge conflict, let me know, but lots of stuff getting done around this fork, kind of exciting, not gonna lie.

If you're offerring, I'd be more than happy to take it. To be honest, since it's an autogenerated file that's not happy I was just going to take all changes from the master branch, then rerun npm install to get things updated for the changes here as needed.

Is there a better way to do it?


But it really is isn't it? I've found I love coming into a repo like this that hasn't seen some attention in a while and getting everything as up to date as I can before I'm done looking at it

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 23, 2023

To be honest, since it's an autogenerated file that's not happy I was just going to take all changes from the master branch, then rerun npm install to get things updated for the changes here as needed.

Well now that you mention, that should just about do it, yeah. I was thinking of all the metadata like the URLs being preserved, but those either aren't in package-lock.json or are consistent in package.json since the changes for this fork were made, or both, so I was kinda overthinking it.

@confused-Techie
Copy link
Member Author

@DeeDeeG okay well in that case, perfect!

But knowing that I've gone ahead and pulled in all changes from master and reran npm install to clear up any inconsistencies in our changes. So we should be good to go on our final PR for this repo, then it's on to updating github, unless of course we would like to bump superstring first, which considering we are on a roll, I'm more than happy to do

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 23, 2023

The superstring thing would help me out a bunch (for progress on building pulsar core repo with Node 18 or newer), it seems very lucky timing that you were already going to do some work on this package.

If it somehow turns into a production, I don't necessarily want to hold up your stuff from getting into github repo, but doing both in one bump at github repo would be snazzy, IMO, or at least holding off before properly rolling another github package release (manually).

(I might wanna make a script that automates the github package transpilation stuff, eventually though. Mildly #off-topic.)

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.

The passing tests keep coming!

I know it's just a baby, but the new spec is already showing its potential! (Or in other words, I'm glad to have something exercising the dugite functionality here!)

Big 👍 for the approach with added tests, I may have suggested it, but you implemented it, so big-time 👍 from me and thanks!

@confused-Techie
Copy link
Member Author

Love the approval, thanks again!

But yes lets keep those passing tests coming, and this repo being up to date is absolutely a team effort, so appreciate you sanity checking my work, and giving awesome ideas.

I'll get this in, then see if I can bump superstring before I'm done for the night, fingers crossed it's as easy as this bump was, since like you said, if we already gotta bump github lets do our work once

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.

None yet

2 participants