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

refactor(gradle): make uri parsing reusable #34722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patrick-dedication
Copy link

@patrick-dedication patrick-dedication commented Mar 10, 2025

Changes

Moves gradle uri parsing to common to make it reusable.

Context

I've got a first walking skeleton for #14208.
I would like to make some preparatory refactorings to make the feature PR concise.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@patrick-dedication patrick-dedication force-pushed the refactor/gradle-make-uri-parsing-reusable branch from cf1a41f to fc21f6d Compare March 10, 2025 05:59
const result = groovy.query(input, qUri, ctx);

expect(result).toMatchObject({
varTokens: [{ value: url }],
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if varTokens is a good "interface".
Should I do storeInTokenMap(ctx, 'url') or storeInTokenMap(ctx, 'uri')?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are already plenty of tests for the different URL patterns that should cover all currently supported combinations. I don't think we need to add them redundantly:

describe('custom registries', () => {
it.each`
def | input | url
${''} | ${'maven("")'} | ${null}
${''} | ${'maven(["https://foo.bar/baz/qux"])'} | ${null}
${''} | ${'maven("foo", "bar")'} | ${null}
${''} | ${'maven("https://foo.bar/baz")'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven("${base}/baz")'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven(base)'} | ${'https://foo.bar'}
${''} | ${'maven(url = "https://foo.bar/baz")'} | ${'https://foo.bar/baz'}
${''} | ${'maven(url = uri("https://foo.bar/baz"))'} | ${'https://foo.bar/baz'}
${''} | ${'maven(uri("https://foo.bar/baz"))'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven(uri("${base}/baz"))'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven(uri(property("base")))'} | ${'https://foo.bar'}
${'base="https://foo.bar"'} | ${'maven(uri(base))'} | ${'https://foo.bar'}
${'base="https://foo.bar"'} | ${'maven(uri(base + "/baz"))'} | ${'https://foo.bar/baz'}
${''} | ${'maven(uri(["https://foo.bar/baz"]))'} | ${null}
${''} | ${'maven { ["https://foo.bar/baz"] }'} | ${null}
${''} | ${'maven { url "https://foo.bar/baz" }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url base + "/baz" }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url "${base}/baz" }'} | ${'https://foo.bar/baz'}
${''} | ${'maven { url uri("https://foo.bar/baz") }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url uri("${base}/baz") }'} | ${'https://foo.bar/baz'}
${''} | ${'maven { url = "https://foo.bar/baz" }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url = "${base}/baz" }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url = property("base") }'} | ${'https://foo.bar'}
${'base="https://foo.bar"'} | ${'maven { url = base }'} | ${'https://foo.bar'}
${''} | ${'maven { url = uri("https://foo.bar/baz") }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url = uri("${base}/baz") }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { url = uri(base) }'} | ${'https://foo.bar'}
${''} | ${'maven { uri(["https://foo.bar/baz"]) }'} | ${null}
${'base="https://foo.bar"'} | ${'maven { name "baz"\nurl = "${base}/${name}" }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { name "${base}" + "/baz"\nurl = "${name}" + "/qux" }'} | ${'https://foo.bar/baz/qux'}
${'base="https://foo.bar"'} | ${'maven { name = "baz"\nurl = "${base}/${name}" }'} | ${'https://foo.bar/baz'}
${'some="baz"'} | ${'maven { name = "${some}"\nurl = "https://foo.bar/${name}" }'} | ${'https://foo.bar/baz'}
${'some="foo.bar/baz"'} | ${'maven { name = property("some")\nurl = "https://${name}" }'} | ${'https://foo.bar/baz'}
${'some="baz"'} | ${'maven { name = some\nurl = "https://foo.bar/${name}" }'} | ${'https://foo.bar/baz'}
${''} | ${'maven { setUrl("https://foo.bar/baz") }'} | ${'https://foo.bar/baz'}
${''} | ${'maven { setUrl(uri("https://foo.bar/baz")) }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { setUrl("${base}/baz") }'} | ${'https://foo.bar/baz'}
${'base="https://foo.bar"'} | ${'maven { setUrl(project.property("base")) }'} | ${'https://foo.bar'}
${'base="https://foo.bar"'} | ${'maven { setUrl(base) }'} | ${'https://foo.bar'}
${''} | ${'maven { setUrl(["https://foo.bar/baz"]) }'} | ${null}
${''} | ${'maven { setUrl("foo", "bar") }'} | ${null}
${'base="https://foo.bar"'} | ${'publishing { repositories { maven("${base}/baz") } }'} | ${null}

Copy link
Author

Choose a reason for hiding this comment

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

For someone new to the codebase like me having these tests helps a lot, because:

  • I can check if my change works on a smaller scale
  • I can pinpoint problems, I had a really hard time finding out where in the whole parser setup things weren't working out for me

@Churro
Copy link
Collaborator

Churro commented Mar 10, 2025

Could you please open a working / draft PR with your implementation of exclusiveContent first?

This would allow a better review of needed refactoring changes. They could then still be split into a separate PR.

@patrick-dedication
Copy link
Author

Could you please open a working / draft PR with your implementation of exclusiveContent first?

This would allow a better review of needed refactoring changes. They could then still be split into a separate PR.

Sure, will do, but that will take a few more days :)
I had the impression this change makes sense anyways?

@Churro
Copy link
Collaborator

Churro commented Mar 10, 2025

Could you please open a working / draft PR with your implementation of exclusiveContent first?
This would allow a better review of needed refactoring changes. They could then still be split into a separate PR.

Sure, will do, but that will take a few more days :) I had the impression this change makes sense anyways?

Without further context, I don't see the value of moving qUri to common.ts, as it is only needed for registry URL parsing and therefore locally scoped. Context-wise a parser definition for exclusiveContent would also fit best into registry-urls.ts

@patrick-dedication
Copy link
Author

patrick-dedication commented Mar 12, 2025

@Churro finally got around to polishing up at least a little.
I will incorporate the changes you made in common.ts when we have discussed the implementation.
Find the draft here #34771, I didn't know how to mark it as draft properly.
More than happy to discuss the implementation :)

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.

2 participants