feat(PurlUtils): Add optional parameters to toPurl
#7601
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7601 +/- ##
============================================
- Coverage 68.03% 68.02% -0.01%
Complexity 2022 2022
============================================
Files 344 344
Lines 16724 16725 +1
Branches 2371 2371
============================================
Hits 11378 11378
- Misses 4363 4365 +2
+ Partials 983 982 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
nnobelis
force-pushed
the
nnobelis/toPurlChanges
branch
from
ec7acb0 to
0f484d6
Compare
|
@sschuberth , mind sharing the rationale for 7da5ae9 ? |
It's the same rational that you follow in similar commits like 38eacac. 0d75760 and others, to reduce visibility of functions that are only supposed to be used internally, to not make too much promises regarding the public API. |
I've been trying to include you into the discussion to figure out whether the rationale for 7da5ae9 does still apply in context of @nnobelis change. The commits of mine are irrelevant in this context. In order to review this I would have liked your view as additional input. But it seems this was not clear to you. |
| @@ -91,7 +101,7 @@ fun Identifier.getPurlType() = | |||
| * [in the documentation](https://github.com/package-url/purl-spec/blob/master/README.rst#purl). | |||
| * E.g. 'maven' for Gradle projects. | |||
| */ | |||
| fun Identifier.toPurl() = if (this == Identifier.EMPTY) "" else createPurl(getPurlType(), namespace, name, version) | |||
| fun Identifier.toPurl() = toPurl(emptyMap(), "") | |||
There was a problem hiding this comment.
Would it make sense to, instead of an overload, just add these two parameter to this function, with default values emptyMap and "" ?
There was a problem hiding this comment.
That's how it was before the force-push, and I also liked that better... @nnobelis why did you change it?
There was a problem hiding this comment.
As explained in the commit message it broke the FreeMarker templates that call toUrl (the tests failed): I guess Kotlin's implementation of extension functions with default values is not compatible with FreeMarker Java's reflection.
There was a problem hiding this comment.
Have you considered annotation the function (from before) with @JvmOverloads? I might solve the problem as well.
There was a problem hiding this comment.
I think we can go around with @JvmOverloads. Would it be ok to put this annotation on toUrl ?
There was a problem hiding this comment.
Where does toUrl now come from? Did you maybe mean this?
@JvmOverloads
fun Identifier.toPurl(qualifiers: Map<String, String> = emptyMap(), subpath: String = "") = ...
There was a problem hiding this comment.
Yes toPurl. Sorry typo.
There was a problem hiding this comment.
If above signature works, IMO it'd bee good.
There was a problem hiding this comment.
@nnobelis, please also explain in the commit message what @JvmOverloads is needed for.
nnobelis
force-pushed
the
nnobelis/toPurlChanges
branch
from
0f484d6 to
5829489
Compare
|
@nnobelis I guess this can be moved out of draft state? |
nnobelis
force-pushed
the
nnobelis/toPurlChanges
branch
from
5829489 to
731a8d6
Compare
The commit 7da5ae9 changed the visibility of `createPurl`, `toPurl` has become the only public ORT function for plugins to generate Purls. Since this function does not support qualifiers nor subpath, these two parameters have to be added to the function signature with default values. The `JvmOverloads` annotation is used to make this function with default parameters compatible with calls coming from FreeMarker templates. Signed-off-by: Nicolas Nobelis <[email protected]>
nnobelis
force-pushed
the
nnobelis/toPurlChanges
branch
from
731a8d6 to
a6f03c3
Compare
|
@fviernau @sschuberth can we please merge this ? |
The commit boschglobal@7da5ae9 changed the visibility of
createPurl,toPurlhasbecome the only public ORT function for plugins to generate Purls.
Since this function does not support qualifiers nor subpath, these two
parameters have to be added to the function signature with default values.