Miscellaneous smaller Pub serialization improvements

[sschuberth]

Please have a look at the individual commit messages for the details.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.49%. Comparing base (ed4bccf) to head (c643da0).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...kage-managers/pub/src/main/kotlin/model/Pubspec.kt	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9287      +/-   ##
============================================
+ Coverage     66.29%   67.49%   +1.20%     
+ Complexity     1201     1198       -3     
============================================
  Files           239      241       +2     
  Lines          8446     8494      +48     
  Branches        905      899       -6     
============================================
+ Hits           5599     5733     +134     
+ Misses         2478     2399      -79     
+ Partials        369      362       -7     

Flag	Coverage Δ
funTest-docker	60.63% <50.00%> (+0.55%)	⬆️
funTest-non-docker	33.63% <ø> (-1.08%)	⬇️
test	37.48% <78.57%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fviernau]

Hosted packages can be represented in multiple ways.
The link above only points to one of the representations.
There is a way to specify a url for hosted dependencies.
So, the data class should IMO remain as-is.

[fviernau]

copied from the docs:

environment:
  sdk: '>=2.14.0 < 3.0.0'

dependencies:
  transmogrify:
    hosted:
      name: transmogrify
      url: https://some-package-server.com
    version: ^1.4.0

[fviernau]

The link you provided points to the short form, which uses a primitive type to specify the url in a more compact way.

[sschuberth]

Hmm. The link IMO documents three forms of hosted packages:

1. With a scalar value:

dependencies:
  transmogrify: ^1.4.0

2. With a map value that contains hosted and version on the same level, while hosted having a scalar value:

dependencies:
  transmogrify:
    hosted: https://some-package-server.com
    version: ^1.4.0

3. With a map value where also hosted is a map (only relevant for an SDK version earlier than 2.15):

dependencies:
  transmogrify:
    hosted:
      name: transmogrify
      url: https://some-package-server.com
    version: ^1.4.0

My goal was to support case 2 where hosted and version are siblings as I did not see this case being covered. Am I missing something?

[sschuberth]

To add to that, the naming in this case actually does not matter too much as these classes are not directly used via their auto-generated serializers, but the DependencyMapSerializer constructs these classes manually, and takes care about the 3. case by explicitly looking for hosted.get<YamlScalar>("url").

Anyway, if the classes were deserialized directly, I believe naming the property hosted instead of url to be more correct.

[fviernau]

I would prefer to keep url, because

1. In the most detailed representation the name is url
2. Hosted is name of the class. So it's kind of Hosted.hosted which seems a tad odd
3. Considering the values, url seems to fit better.
4. It's debatable whether the version values are actually part of the hosted object. It seems to rather be a sibling. Anyhow, it's in the same data class.

[sschuberth]

I still don't believe it makes sense to stick with url in the longer term as it would break as soon as we'd be able to deserialize classes directly (see charleskorn/kaml#29 and / or charleskorn/kaml#612), but for the sake of moving things forward I'll drop that commit.

[fviernau]

Hm, personally sacrificing encapsulation for these few bytes of memory is not worth it.
What do you think about the middle ground of keeping it private, but turning it into a function?

[sschuberth]

Why would encapsulation even be required here? I mean, these two files are supposed to be deserialized in exactly the same way, so it makes sense and is correct to share the same Yaml instance, or?

Background: I was originally doing this in the context of adding more tests for deserialization (which turned out to be unnecessary). As part of that, I needed access to YAML, but I couldn't make either instance internal as it would then have conflicted with the other private one.