Skip to content

Conversation

KickNext
Copy link

@KickNext KickNext commented Jan 30, 2025

Summary

Replaced pubspec_yaml with yaml for parsing pubspec.yaml, as pubspec_yaml has not been updated for three years and breaks when parsing pubspec.yaml files containing modern dependency imports from external pub servers with the hosted parameter.

Reproduction Example

To reproduce the issue, add the following dependency to pubspec.yaml:

isar_generator:
  hosted: https://pub.isar-community.dev
  version: 3.1.8

The pubspec_yaml parser fails to handle this format correctly.

Additional Fixes

  • Improved version parsing. Previously, when running locally on Windows, the extracted version string included an unwanted path at the end. This issue has been resolved.

@Pebkac03
Copy link
Contributor

Did you manage to identify why the issues descibed in additional fixes occurred? I've found there are a few instances where using windows causes problems (see #52). replaceAll('"', '') certainly works but feel we should, if the problem exists in the projects own code, have it as a goal to solve the cause eventually.

Also, are there any tests you feel should be added?

@KickNext
Copy link
Author

KickNext commented Feb 10, 2025

@Pebkac03 I spent an hour trying to reproduce the issue, and it seems I just overdid it. The extra " appeared in package in PubService because it takes the form
stacked_cli 1.13.4 at path "D:\Hob\my\cli" when locally activated.

As a result, I encountered a parsing error since package.split(' ').last returned the path with ".

In all the scenarios I came across, workingDirectory in PubspecService was always null.
So .replaceAll('"', '').trim() is overengineering, and I’m not sure it’s necessary—it could be removed.

Regarding tests: it might make sense to fetch the latest pubspec.yaml with all possible rule variations.
However, since only one field is used and the Yaml parser works as a Map, I think it’s not necessary.

I didn’t generate tests since they are generated in Actions.

Without this fix, my team and I can't use the latest version of stacked, so I’m happy to assist promptly.

Upd: ArgResults removes the first layer of quotes from what goes into rest, and that's exactly where we get workingDirectory from, so this is unnecessary.

Stacked-Org/stacked#1146 Very similar to the same thing, package.split(' ').last not version.

@KickNext
Copy link
Author

@FilledStacks @Pebkac03 I have rebuilt the branch on the latest version without unnecessary history and without modifying version parsing, as it has been properly fixed in commit 5c21c57. I didn't consider that the version number could be more complex

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