Skip to content

Conversation

@sodic
Copy link
Contributor

@sodic sodic commented Feb 12, 2025

I had this on my refactoring list. PackageJson is a part of the AppSpec but was in ExternalConfig, which means AppSpec depended on ExternalConfig.

On the other hand, Dependency was part of AppSpec, so ExternalConfig also depends on AppSpec

This PR introduces a new Npm module and fixes the circular dependency.

@sodic sodic self-assigned this Feb 18, 2025
@Martinsos
Copy link
Member

We can merge this one, right?

@sodic sodic changed the title Move PackageJson to AppSpec Move PackageJson and Npm dependency moduels Apr 2, 2025
@sodic sodic changed the title Move PackageJson and Npm dependency moduels Refactor PackageJson and Npm dependency moduels Apr 2, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was completely unused.

Funnily enough, one module did use the printDep function but it defined it locally (didn't import this one) 😄

@Martinsos Martinsos self-requested a review April 2, 2025 13:16
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is much better! Some small things, I commented on them.
Btw it would have helped in a PR like this with ton of files that have nothing but changes in the imports, if you said in the main comment that most of the changes are like that, and that you commented on specific files which are more interesting to look at. That would help me go fast through some files and focus on others, while now I had to focus on each one file not to miss anything, and still I almost missed some stuff because it all looks the same after Nth file hah.
EDIT: you did comment on some though, I am aware of that! Just I didn't know if you commented on all, so I had again to be very careful.

@sodic
Copy link
Contributor Author

sodic commented Apr 4, 2025

Btw it would have helped in a PR like this with ton of files that have nothing but changes in the imports, if you said in the main comment that most of the changes are like that

My bad, it used to be clear from the PR's name but then I changed it (for the worse it seems).

@sodic sodic changed the title Refactor PackageJson and Npm dependency moduels Create an Npm module and move PackageJson and Dependency there Apr 4, 2025
@sodic sodic changed the title Create an Npm module and move PackageJson and Dependency there Create an Npm module and move PackageJson and Dependency there Apr 4, 2025
@sodic sodic merged commit 101919b into main Apr 8, 2025
6 checks passed
@sodic sodic deleted the filip-move-packagejson branch April 8, 2025 11:02
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.

4 participants