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.

Sign up for GitHub

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

Jump to bottom

Validate dependencies and incompatibilities #91

Merged
merged 5 commits into from
Feb 5, 2023
Merged

Validate dependencies and incompatibilities #91

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00990c8
validate dependencies and incompatibilities (closes #23)
ithinkandicode Feb 4, 2023
24196d6
Merge branch 'develop' into validate-dependencies-and-incompatibilities
ithinkandicode Feb 4, 2023
7894de6
Merge branch 'develop' into validate-dependencies-and-incompatibilities
ithinkandicode Feb 4, 2023
fb10b40
dependency validation: use `get_mod_id`
ithinkandicode Feb 5, 2023
6c5a156
shorten `is_dependency_or_incompatibility_valid` to `is_mod_id_valid`…
ithinkandicode Feb 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions addons/mod_loader/mod_manifest.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ func _init(manifest: Dictionary) -> void:
tags = _get_array_from_dict(godot_details, "tags")
config_defaults = godot_details.config_defaults

var mod_id = get_mod_id()
if not validate_dependencies_and_incompatibilities(mod_id, dependencies, incompatibilities):
return

# todo load file named icon.png when loading mods and use here
# image StreamTexture

Expand Down Expand Up @@ -136,6 +140,55 @@ static func is_semver_valid(check_version_number: String) -> bool:
return true


static func validate_dependencies_and_incompatibilities(mod_id: String, dependencies: PoolStringArray, incompatibilities: PoolStringArray) -> bool:
var valid_dep = true
var valid_inc = true

if dependencies.size() > 0:
for dep in dependencies:
valid_dep = is_mod_id_valid(mod_id, dep, "dependency")

if incompatibilities.size() > 0:
for inc in incompatibilities:
valid_inc = is_mod_id_valid(mod_id, inc, "incompatibility")

if not valid_dep or not valid_inc:
return false

return true


static func is_mod_id_valid(original_mod_id: String, check_mod_id: String, type := "") -> bool:
var intro_text = "A %s for the mod '%s' is invalid: " % [type, original_mod_id] if not type == "" else ""

# contains hyphen?
if not check_mod_id.count("-") == 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could use the is_name_or_namespace_valid func if you String.split("-") here (and checked if the array length is exactly 2)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried this originally, but it wasn't nearly as helpful. Here's the error if I use is_name_or_namespace_valid:

error-ste

And here's the error message with the approach I've used:

error-current

The 1st error tells you something is wrong with either a name or a namespace, but gives no indication of where/how to fix the problem. You don't know if it's occurring in your own mod or a dependency of it, nor whether it's an issue with the mods own name, or a dependency, or an incompatibility.

Whereas the 2nd error tells you which mod has an issue, and exactly where the issue is.

Trying to use is_name_or_namespace_valid initially is actually the reason I made my errors so explicitly helpful: When I was testing this I made a bunch of purposefully faulty manifests, but found that I had no idea which of them were incorrect.

Better to do as much as we can to help people fix issues like this IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, you are right. Then it might be a better idea to turn the log in is_name_or_namespace_valid into an error and add the log fatal like you did here once it returns false. I think that avoids having two places for id validation and the logs are still as informative, right? And don't forget to check false and log fatal after the other uses of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe that would be better in a subsequent PR? I don't want to change anything beyond the scope of validating deps/incompats for this one

Copy link
Collaborator

Choose a reason for hiding this comment

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

to get it done with I suppose. Having two places for the same validation is not good though, that needs to change

ModLoaderUtils.log_fatal(str(intro_text, 'Expected a single hypen in the mod ID, but the %s was: "%s"' % [type, check_mod_id]), LOG_NAME)
return false

# at least 7 long (1 for hyphen, 3 each for namespace/name)
var mod_id_length = check_mod_id.length()
if mod_id_length < 7:
ModLoaderUtils.log_fatal(str(intro_text, 'Mod ID for "%s" is too short. It must be at least 7 characters, but its length is: %s' % [check_mod_id, mod_id_length]), LOG_NAME)
return false

var split = check_mod_id.split("-")
var check_namespace = split[0]
var check_name = split[1]
var re := RegEx.new()
re.compile("^[a-zA-Z0-9_]*$") # alphanumeric and _

if re.search(check_namespace) == null:
ModLoaderUtils.log_fatal(str(intro_text, 'Mod ID has an invalid namespace (author) for "%s". Namespace can only use letters, numbers and underscores, but was: "%s"' % [check_mod_id, check_namespace]), LOG_NAME)
return false

if re.search(check_name) == null:
ModLoaderUtils.log_fatal(str(intro_text, 'Mod ID has an invalid name for "%s". Name can only use letters, numbers and underscores, but was: "%s"' % [check_mod_id, check_name]), LOG_NAME)
return false

return true


# Returns an empty String if the key does not exist
static func _get_string_from_dict(dict: Dictionary, key: String) -> String:
if not dict.has(key):
Expand Down