Skip to content
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

Add better logging for Wally packages in the wrong dependency organization #83

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FxllenCode
Copy link

@FxllenCode FxllenCode commented May 3, 2022

Done:

  • Basic implementation of the idea
  • Preparation for more integration

Todo:

  • Improve the logging to mock the Wally.toml format
  • Possibility add a feature to get the package's intended realm (?).
  • Update tests

@FxllenCode
Copy link
Author

p.s. - I am aware that the implementation right now has a flaw!

i.e. if someone has a dev-dependency inside dependencies, it will still suggest to put it under server dependencies, which is not good behavior.

To solve this, I would like to get the realm of the package trying to be installed, I don't really know how to go about doing that but that is my plan to make it more usable (even if this requires a minor refactor of package_req).

Cheers! 🎉

@FxllenCode
Copy link
Author

@magnalite still would like to get some advice so I can finish this PR - suggestions?

@magnalite
Copy link
Member

Sorry for taking so long to get to this PR! The only invalid realm conflicts are

  • a server package being place in shared dependencies
  • a dev dependency being placed anywhere not in dev dependencies

I agree the best way to present this to the user is to detect which realm the package is supposed to be in and present them with a suggestion for that. This can be achieved by getting the PackageMetadata from the PackageIndex. I'm not sure what the most ergonomic way to add this data would be but it may be easiest to add a mapping of package -> index in the PackageSourceMap that gets generated.

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