Skip to content

Improve directory name extraction from branch name.#495

Closed
dreid wants to merge 3 commits intodependabot:mainfrom
dreid:dreid-improve-directory-extraction
Closed

Improve directory name extraction from branch name.#495
dreid wants to merge 3 commits intodependabot:mainfrom
dreid:dreid-improve-directory-extraction

Conversation

@dreid
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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