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

Create mod entry in enabledmods.json if it doesn't exist #410

Merged
merged 15 commits into from
Oct 8, 2023

Conversation

Alystrasz
Copy link
Contributor

Currently, when you add a new mod to your mods/ directory, when there's no associated entry in the enabledmods.json file, it is considered enabled by default; mod entries are only written in enabledmods.json when toggling them via the mods interface.

In this pull request, I propose to create mod entries in enabledmods.json when detecting they don't exist.

While loading mods, if there's no related entry in enabledmods.json
file, we create one with value=true, activating new mods by default.
@Alystrasz Alystrasz mentioned this pull request Jan 30, 2023
8 tasks
@BobTheBob9
Copy link
Member

will this write downloaded mods to the json as well? while we don't have autodownloading yet we do read the mod directory for downloaded mods atm, and i don't think we should ever write downloaded mods to the json

@Alystrasz
Copy link
Contributor Author

will this write downloaded mods to the json as well? while we don't have autodownloading yet we do read the mod directory for downloaded mods atm, and i don't think we should ever write downloaded mods to the json

Fixed in 5de25eb and 21975fa

@Alystrasz Alystrasz requested review from a team and BobTheBob9 February 23, 2023 23:47
@F1F7Y
Copy link
Member

F1F7Y commented Jun 28, 2023

This would be pretty pog ngl

@F1F7Y F1F7Y added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Jun 28, 2023
@Alystrasz Alystrasz requested review from F1F7Y and removed request for a team and BobTheBob9 June 28, 2023 22:07
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

Works in testing

@Alystrasz
Copy link
Contributor Author

I won't edit the labels for this PR, since I made it, but I'd say this is ready to be merged. :)

@F1F7Y F1F7Y added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Jun 29, 2023
Jan200101
Jan200101 previously approved these changes Jul 4, 2023
@GeckoEidechse GeckoEidechse dismissed Jan200101’s stale review July 26, 2023 20:51

missing comment in approving review

@Alystrasz Alystrasz added READY TO MERGE This mergeable right now and removed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Aug 3, 2023
Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

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

re-doing the review
code still looks good and it matches its surrounding code

@GeckoEidechse GeckoEidechse changed the title fix: Create mod entry if it doesn't exist Create mod entry in enabledmods.json if it doesn't exist Oct 8, 2023
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Code looks fine as far as I can tell. Primarily looked at control flow.

@GeckoEidechse GeckoEidechse merged commit a040bff into R2Northstar:main Oct 8, 2023
2 checks passed
@Alystrasz Alystrasz deleted the fix/new-mod-entries branch October 9, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants