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

dev: Packages #429

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

dev: Packages #429

wants to merge 42 commits into from

Conversation

GeckoEidechse
Copy link
Member

@GeckoEidechse GeckoEidechse commented Jul 18, 2023

This PR is just to see the diff between all missing packages related changes and main

It contains

As such this PR should stay draft and not be merged on its own but rather to be used as a PoC and see overall progress of implementing the packages format.

@GeckoEidechse
Copy link
Member Author

So this is working now in testing. The code is a bit messy and partially duplicated but as I mentioned before I'm time limited and this works fine for the release. Once players start transitioning over to packages we can clean up a bunch of things 👀

@GeckoEidechse
Copy link
Member Author

Also in the "upgrade to packages" part I should probably also add a sort of exception for mod settings where in the upgrade process mod settings gets skipped so it just gets deleted instead as it's not in the game anyway.

@GeckoEidechse
Copy link
Member Author

Also I'd say the release that has everything packages related merged should be a 2.0 ^^

Copy link
Contributor

@catornot catornot left a comment

Choose a reason for hiding this comment

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

I just like to complain about rust code.
also pls pls just actually use the result enum instead of unwrapping everything.

Comment on lines 472 to 492
let mut has_mods = false;
let mut mod_json_exists = false;

// Checks for `mods/*/mod.json`
for i in 0..archive.len() {
let file = match archive.by_index(i) {
Ok(file) => file,
Err(_) => continue,
};
let file_path = file.mangled_name();
if file_path.starts_with("mods/") {
has_mods = true;
if let Some(name) = file_path.file_name() {
if name == "mod.json" {
let parent_path = file_path.parent().unwrap();
if parent_path.parent().unwrap().to_str().unwrap() == "mods" {
mod_json_exists = true;
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is literately the perfect use for a iterator.
no mutability :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, I'm dumb, can I have an example? ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

smth like this
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=93be605e794e1939f0deeaeebb767575

it might not compile but it shouldn't be hard to fix stuff in it I think.

Comment on lines 442 to 458
for directory in directories {
let folder_name = directory.file_name().unwrap().to_str().unwrap();
let ts_mod_string_from_folder: ParsedThunderstoreModString = match folder_name.parse() {
Ok(res) => res,
Err(err) => {
log::warn!("{err}");
continue;
}
};
// Check which match `AUTHOR-MOD` and do NOT match `AUTHOR-MOD-VERSION`
if ts_mod_string_from_folder.author_name == thunderstore_mod_string.author_name
&& ts_mod_string_from_folder.mod_name == thunderstore_mod_string.mod_name
&& ts_mod_string_from_folder.version != thunderstore_mod_string.version
{
delete_package_folder(&directory.display().to_string())?;
}
}
Copy link
Contributor

@catornot catornot Jul 21, 2023

Choose a reason for hiding this comment

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

a iterator would also be better here although you can probably keep it and just move all these if cases into filters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah so I kinda merged #426 before seeing your comments on here. Will do some refactoring to change update to your feedback though ^^

Comment on lines 142 to 160
for legacy_mod in found_installed_legacy_mods {
if legacy_mod.thunderstore_mod_string.is_none() {
continue; // Not a thunderstore mod
}

let current_mod_ts_string: ParsedThunderstoreModString = legacy_mod
.clone()
.thunderstore_mod_string
.unwrap()
.parse()
.unwrap();

if thunderstore_mod_string.author_name == current_mod_ts_string.author_name
&& thunderstore_mod_string.mod_name == current_mod_ts_string.mod_name
{
// They match, delete
delete_mod_folder(&legacy_mod.directory)?;
}
}
Copy link
Contributor

@catornot catornot Jul 21, 2023

Choose a reason for hiding this comment

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

a iterator would also be better here although you can probably keep it and just move all these if cases into filters.

};

// Successful package install
match legacy::delete_legacy_package_install(thunderstore_mod_string, game_install) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a if let here

}
};

match delete_older_versions(thunderstore_mod_string, game_install) {
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

Comment on lines 433 to 440
for path in paths {
let my_path = path.unwrap().path();

let md = std::fs::metadata(my_path.clone()).unwrap();
if md.is_dir() {
directories.push(my_path);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for loop found, use a iterator instead in this case. ( I hate for loops ).

Comment on lines +103 to +112
// This is a stupid way to show a legacy installed mod as outdated by simply giving back a wrong version number
if thunderstore_mod_string.is_some() {
// Parse the string
let mut parsed_string: ParsedThunderstoreModString =
thunderstore_mod_string.clone().unwrap().parse().unwrap();
// Set version number to `0.0.0`
parsed_string.version = "0.0.0".to_string();
// And store new string back in original variable
thunderstore_mod_string = Some(parsed_string.to_string())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps just move this into the match case above ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't I have to duplicate the code then? Cause the match matches for like two cases

  1. checking if it's in mods.json
  2. checking if it there's a manifest.json and a thunderstore_author.txt

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.

None yet

2 participants