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

bloodhunter: Fix patron for Order of the Profane Soul #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PJBrs
Copy link
Contributor

@PJBrs PJBrs commented Jul 11, 2024

The Bloodhunter Feature Selector for OtherwordlyPatron lists the options for the patron choices capitalised. However, the code in FeatureSelector expects them to be lowercased. Make the options lowercase.

An alternative, more robust fix might have been to change some stuff over here:

if selection.lower() in t.options:

So, the user's selection in feature_choices is lowered, but it is not checked against a lowered t.options in FeatureSelector. Then again, I think it only should check against the keys of t.options in FeatureSelector... Anyway, that's all academic for now, this patch just fixes #134

Usually, I'd have added c868575 as well, which is a good way to test this fix, but this time I decided to err on the side of caution :-)

@PJBrs
Copy link
Contributor Author

PJBrs commented Jul 11, 2024

Just to add the other solution:

diff --git a/dungeonsheets/features/features.py b/dungeonsheets/features/features.py
index 399dded..74b5010 100644
--- a/dungeonsheets/features/features.py
+++ b/dungeonsheets/features/features.py
@@ -89,10 +89,12 @@ class FeatureSelector(Feature):
         new_feat.source = t.source
         new_feat.needs_implementation = True
         for selection in feature_choices:
-            if selection.lower() in t.options:
-                feat_class = t.options[selection.lower()]
-                if owner.has_feature(feat_class):
-                    continue
-                new_feat = feat_class(owner=owner)
-                new_feat.source = t.source
+            for k in t.options.keys():
+                if selection.lower() == k.lower():
+                    feat_class = t.options[k]
+                    if owner.has_feature(feat_class):
+                        continue
+                    new_feat = feat_class(owner=owner)
+                    new_feat.source = t.source
+                    break
         return new_feat

This way, it doesn't matter whether the options in a FeatureSelector are capitalised or lowercase. But don't let that refrain you from merging this PR!

Copy link
Owner

@canismarko canismarko left a comment

Choose a reason for hiding this comment

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

Prefer the alternate fix that changes the matching.

This patch makes parsing options for a FeatureSelector
more robust. First, it only compares selections with
the keys of the FeatureSelector options dictionary,
and second, it doesn't care whether developers have
defined options lowercase or uppercase.

In so doing, this fixes selecting a patron for the
bloodhunter Order of the Profane Soul:

The Bloodhunter Feature Selector for OtherwordlyPatron
lists the options for the patron choices capitalised.
However, the code in FeatureSelector expected them to
be lowercased. Make the FeatureSelector not care about
case.
@PJBrs
Copy link
Contributor Author

PJBrs commented Aug 23, 2024

Switched to the alternative fix!

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.

Specifying Bloodhunter's Patron
2 participants