Warn error options invalid when warn or error set to none #10453
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10453 +/- ##
==========================================
+ Coverage 88.76% 88.79% +0.03%
==========================================
Files 180 180
Lines 22564 22562 -2
==========================================
+ Hits 20029 20035 +6
+ Misses 2535 2527 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
First, thank you for doing this work ❤️ This is well done 🙂 However, I worry the changes as is if merged would remove a generalized utility that we'll likely continually need.
Specifically, exclusive_primary_alt_value_setting was created specifically to handle general name changes gracefully. Although it is only being used for exclude/warn and include/error at this time, it is not unlikely that there will be other keys we want to rename in the future. As such, altering the function to only work with warn_error_options seems like a step back.
I think there are two simpler changesets to fix the problem we are trying to address: extending exclusive_primary_alt_value_setting to default the associated value OR create an orchestrating function.
Alt (1): extending exclusive_primary_alt_value_setting
First the function would take an additional optional argument making the function signature look like
def exclusive_primary_alt_value_setting(
dictionary: Optional[Dict[str, Any]],
primary: str,
alt: str,
parent_config: Optional[str] = None,
default_value: Optional[Any] = None,
) -> None:Then at the end of the function we'd conditionally set the default via
if dictionary.get(primary) is None:
dictionary[primary] = default_valueAlt (2): Orchestrating Function
If we didn't want to modify exclusive_primary_alt_value_setting at all, then we could put the defaulting logic into an orchestrating function specific for warn_error_options munging. Somthing like
def normalize_warn_error_options(dictionary: Dict[str, Any]) -> None:
exclusive_primary_alt_value_setting(
warn_error_options, "include", "error", "warn_error_options"
)
exclusive_primary_alt_value_setting(
warn_error_options, "exclude", "warn", "warn_error_options"
)
if dictionary.get("include") is None:
dictionary["include"] = []
if dictionary.get("exclude") is None:
dictionary["exclude"] = []
|
I see that you intended "exclusive_primary_alt_value_setting" as a general purpose utility, but it looks like premature generalization to me. What I've found is that when/if you actually get to some other use case that falls kind of in the same category, it will have other constraints or conditions and the intended "general" utility won't actually work at all without complicating changes. I think it makes more sense to derive a general use case utility when there is more than one use case. |
I'm not sure that I agree that it's a pre-mature generalization, but I'm absolutely open to discussing it further. Can you talk more about your concern that makes the existing function pre-mature? In my mind, there is more than one use case. The existing function addresses the use case of "A key in a dictionary is being renamed to another, but we want to support both as inputs". It is being used for two keys. It renames "error" to "include" and "warn" to "exclude". I do agree that there could be more constraints. The issue that this PR aims to address is one of those, the ability to have a default value association. This could simply be addressed as outlined above, extending the existing function or external to it. There are additional constraints that could be imagined, such as supporting multiple "alt" key names for a given "primary". I don't recommend we do work to handle multiple "alts" as we don't have an existing or planned use case where that is necessary. |
|
From your comment and my comments, my understanding is that we view the underlying discrete cases differently. I also think both your view and my view are valid. The work I did on renaming dictionaries keys in dictionaries has more than one use case. However, we need to default all the values for keys in def normalize_warn_error_options(
dictionary: Dict[str, Any],
) -> None:
convert = {
"error": "include",
"warn": "exclude",
"silence": None,
}
for key, alt in convert.items():
if alt is not None:
exclusive_primary_alt_value_setting(dictionary, key, alt, "warn_error_options")
if dictionary.get(key) is None:
dictionary[key] = [] |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.8.latest 1.8.latest
# Navigate to the new working tree
cd .worktrees/backport-1.8.latest
# Create a new branch
git switch --create backport-10453-to-1.8.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 33161a3035d965e43642dbcafa97b9ecc95ce384
# Push it to GitHub
git push --set-upstream origin backport-10453-to-1.8.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.8.latestThen, create a pull request where the |
…10453) * Fix exclusive_primary_alt_value_setting to set warn_error_options correctly * Add test * Changie * Fix unit test * Replace conversion method * Refactor normalize_warn_error_options
resolves #10452
Problem
If a field in warn_error_options was set to None, a valid WarnErrorOptions object was not constructed.
Solution
Fix the function to normalize the warn_error_options dictionary.
Checklist