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

Improve directory name extraction from branch name. #495

Closed

Conversation

dreid
Copy link

@dreid dreid commented Feb 12, 2024

Fixes #493
Fixes #494

This improves directory extraction to handle two previously unhandled cases:

  1. a multi-segment directory with a non-standard separator is now properly reconstructed by joining on /
  2. the - included in the depname-version portion is properly accounted for when the delimiter is -.

I did not address the fact that the existing non-standard separator test uses a separator (|) that does not appear to be valid according to the dependabot documention.

@dreid dreid force-pushed the dreid-improve-directory-extraction branch from a4a8652 to 66389d7 Compare April 1, 2024 22:14
@jeffwidman
Copy link
Member

👋 thanks for this! I'm on paternity leave right now so hesitant to merge this until I return, as I don't want to risk any problems that others would have to cleanup... but when I return in June I plan to take a look at this and hopefully ship it.


if (data['updated-dependencies']) {
return await Promise.all(data['updated-dependencies'].map(async (dependency, index) => {
const dirname = `/${chunks.slice(2, -1 * (1 + (dependency['dependency-name'].match(/\//g) || []).length)).join(delim) || ''}`
const dirname = `/${chunks.slice(2, -1 * (delimIsDash ? 2 : 1 + (dependency['dependency-name'].match(/\//g) || []).length)).join('/') || ''}`
Copy link
Member

Choose a reason for hiding this comment

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

Hi I'm trying to wrap my head around this change. Would you mind explaining how it works and what featch-metadata was doing before?

Also, I'm surprised that we didn't have to update any existing tests! I guess we didn't have any test cases for separators?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I just saw your examples in #493 and #494 !

So I'm guessing this change makes fetch-metadata not include the extra word after the separator. That makes sense!

@Nishnha
Copy link
Member

Nishnha commented Apr 24, 2024

There are some merge conflicts that you will need to resolve by rebasing/merging and re-running the build script before I can merge this

Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

The change lgtm but there are some merge conflicts that will need to be resolved before it can be pulled in

@dreid
Copy link
Author

dreid commented Apr 30, 2024

@Nishnha These cases appear to be covered by the refactoring of directory detection that landed in a44a9df

@sachin-sandhu
Copy link

Hi @dreid ,
Can you please resolve the merge conflicts so that we may proceed to complete the code merge.

Conflicting files
dist/index.js
src/dependabot/update_metadata.ts

@dreid
Copy link
Author

dreid commented May 20, 2024

I believe these underlying issues were already fixed by a44a9df

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants