-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: main
Are you sure you want to change the base?
refactor(gradle): make uri parsing reusable #34722
Conversation
cf1a41f
to
fc21f6d
Compare
const result = groovy.query(input, qUri, ctx); | ||
|
||
expect(result).toMatchObject({ | ||
varTokens: [{ value: url }], |
There was a problem hiding this comment.
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')
?
There was a problem hiding this comment.
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:
renovate/lib/modules/manager/gradle/parser.spec.ts
Lines 659 to 703 in 0ad6d0a
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} |
There was a problem hiding this comment.
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
Could you please open a working / draft PR with your implementation of 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 :) |
Without further context, I don't see the value of moving |
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])
How I've tested my work (please select one)
I have verified these changes via: