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

feat: Support reading mods from Northstar packages directory #417

Merged
merged 6 commits into from
Jul 19, 2023

Conversation

GeckoEidechse
Copy link
Member

@GeckoEidechse GeckoEidechse commented Jul 16, 2023

TODO:

  • Deleting Northstar mod does not clean up entire packages dir after deletion
    On second thought, this is not really an issue. Might add a cleanup function for this later though.
  • Code cleanup

Part of #415

@GeckoEidechse GeckoEidechse changed the title feat: Initial support for reading NS packages dir feat: Support reading mods from Northstar packages directory Jul 17, 2023
@GeckoEidechse GeckoEidechse marked this pull request as ready for review July 17, 2023 20:10
@Alystrasz
Copy link
Contributor

A few questions, for context, before I review:

@GeckoEidechse
Copy link
Member Author

Exactly :D

  • Why do I feel like the code was copy-pasted from an already-existing module, while (in my mind) reading mods from a new directory should "simply" be done by adding a reference to previously-mentioned directory?

Cause it partially is. I'd have to do a bunch of refactoring before I can re-use the existing code, so I opted to just duplicating. Not the nicest but the fastest way to get it working. I'm planning to do it properly later but I'm too time restricted atm and the Northstar update is planned to be dropped this weekend so I want to get out support before that ^^

Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Code is good and works on my Windows 10 machine.

image

@GeckoEidechse GeckoEidechse merged commit 2ad7807 into main Jul 19, 2023
@GeckoEidechse GeckoEidechse deleted the feat/read-packages-dir branch July 19, 2023 14:58
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