Pub: Parse source artifacts for hosted packages #8178
Conversation
mnonnenmacher
force-pushed
the
pub-source-artifacts
branch
from
434f6c6 to
5dcb946
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8178 +/- ##
=========================================
Coverage 67.16% 67.16%
Complexity 2049 2049
=========================================
Files 357 357
Lines 17158 17158
Branches 2461 2461
=========================================
Hits 11525 11525
Misses 4611 4611
Partials 1022 1022
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -532,8 +532,10 @@ class Pub( | |||
| var vcs = VcsInfo.EMPTY | |||
| var authors = emptySet<String>() | |||
|
|
|||
| val source = pkgInfoFromLockFile["source"].textValueOrEmpty() | |||
There was a problem hiding this comment.
How about doing the same for pkgInfoFromLockFile["description"].textValueOrEmpty(), too, while at it?
There was a problem hiding this comment.
On a second thought, maybe just extract val description = pkgInfoFromLockFile["description"] so you can also make use of it in your last commit.
There was a problem hiding this comment.
The reason for not doing so is probably because there is already a var description, see line 529.
There was a problem hiding this comment.
Right... but that could easily be solved by using a different variable name, like descriptionNode. Anyway, it was just a proposal.
There was a problem hiding this comment.
I think the whole function needs a refactoring, especially the large when condition. But I don't have time to look deeper into it, esp. since the test data is incomplete, e.g. there are no tests for the host == "git" case, so I only did the small change that was useful for my change.
...kage-managers/pub/src/funTest/assets/projects/synthetic/pub-expected-output-multi-module.yml
Show resolved
Hide resolved
| @@ -613,8 +626,7 @@ class Pub( | |||
| homepageUrl = homepageUrl, | |||
| // Pub does not create binary artifacts, therefore use any empty artifact. | |||
| binaryArtifact = RemoteArtifact.EMPTY, | |||
| // Pub does not create source artifacts, therefore use any empty artifact. | |||
| sourceArtifact = RemoteArtifact.EMPTY, | |||
| sourceArtifact = sourceArtifact, | |||
There was a problem hiding this comment.
What about the original comment here, was it wrong? Is it worth mentioning that in the commit message?
There was a problem hiding this comment.
I'm adding support for source artifacts, isn't it obvious that the sentence was wrong?
There was a problem hiding this comment.
The comment said "Pub does not create source artifacts" which to me sounded like Pub does not have the concept of source artifacts at all, so I was surprised that you can add support for them.
There was a problem hiding this comment.
TBH, I have no idea why that sentence was added in the first place, maybe it was wrong all the time, maybe it was correct back in 2019 when it was added.
This is a fixup for e7bdb21. Signed-off-by: Martin Nonnenmacher <[email protected]>
The expected test results became outdated as these tests are currently disabled on CI. Signed-off-by: Martin Nonnenmacher <[email protected]>
Extract the result of reading the package source into a variable to reduce code duplication. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
pub-source-artifacts
branch
from
5dcb946 to
3775604
Compare
Parse source artifact information for packages that are hosted in a package registry like pub.dev from the lockfile. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
pub-source-artifacts
branch
from
3775604 to
5f72e01
Compare
Add support for source artifacts for hosted packages. See the commit messages for details.