Only filter by app & title keys by default #100
Conversation
|
This pull request introduces 1 alert when merging 4206a8d into b11fbe0 - view on LGTM.com new alerts:
|
| @@ -15,7 +15,7 @@ class Rule: | |||
| ignore_case: bool | |||
|
|
|||
| def __init__(self, rules: Dict[str, Any]): | |||
| self.select_keys = rules.get("select_keys", None) | |||
| self.select_keys = rules.get("select_keys", ["app", "title"]) | |||
There was a problem hiding this comment.
This would be better as a default when creating a new rule in the frontend, not made an implicit default in the transform imo.
There was a problem hiding this comment.
It doesn't look like there is an existing component we can use for a multiselect/array form input right now. Am I missing something?
Here's what I propose here:
- Merge this PR to avoid accidental categorization (matching against
urlcould yield bad results) - Having an explicit
allmagic key will be helpful regardless - Work on getting a array input +
select_keysavailable on the frontend
There was a problem hiding this comment.
It doesn't look like there is an existing component we can use for a multiselect/array form input right now.
No, but should be easy to add. Could even just put a text field, and ask users to comma-separate.
|
@ErikBjare What do you think about updating the defaults of all existing rules with |
|
This pull request introduces 1 alert when merging 876b32c into 5bd84b1 - view on LGTM.com new alerts:
|
If we added a way in the web-ui for the user to select the keys himself in a user-friendly way that would probably be OK for most rules. If it's a hidden configuration that the user can't change without exporting and then importing the rules again however I don't think it's a good idea to hide the behavior from the user. |
|
Agreed, my assumption here is we would add this to the webui at some point soon. In the meantime, I'm wondering if it makes sense to add defaults to each default rule as I believe these are written to only match against the app & title keys. Introducing more keys may cause categorization errors. |
iloveitaly
force-pushed
the
use-default-select-keys
branch
from
876b32c
to
b085d42
Compare
|
This pull request introduces 1 alert when merging cbdd792 into 8aaa353 - view on LGTM.com new alerts:
|
@ErikBjare what do you think here? |
Matching against url, editor payload keys, etc could be problematic. The existing rules are written against these two fields, as far as I can tell.
iloveitaly
force-pushed
the
use-default-select-keys
branch
from
340c6c1
to
35cb344
Compare
|
@iloveitaly Just keep the old behavior (server-side default to check all keys, undo beac24b), it's good enough for now. |
Matching against url, editor payload keys, etc could be problematic. The existing rules are written against these two fields, as far as I can tell.