Optionally disable sortand and deduplication of wildcards #89
Conversation
|
This is a pretty simple change. I would prefer to provide these settings in the constructor rather than setter methods, but until we can be confident that the extension will update the dynamicprompts library, I am hesitant to change method signatures. Thoughts? |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
=======================================
Coverage 97.66% 97.67%
=======================================
Files 70 70
Lines 2864 2876 +12
=======================================
+ Hits 2797 2809 +12
Misses 67 67
☔ View full report in Codecov by Sentry. |
adieyal
force-pushed
the
feature/disable-sorting-and-deduplication
branch
from
822804c
to
3783f58
Compare
| def set_sort_wildcards(self, value: bool): | ||
| """ | ||
| Set whether wildcard values should be sorted. | ||
| """ | ||
| self._sort_wildcards = value | ||
|
|
||
| def set_dedup_wildcards(self, value: bool): | ||
| """ | ||
| Set whether wildcard values should be deduplicated. | ||
| """ | ||
| self._dedup_wildcards = value |
There was a problem hiding this comment.
def set_... doesn't smell very Pythonic. Maybe these should just be kwargs on WildcardManager's constructor, and the properties don't need to be _private either?
There was a problem hiding this comment.
I should have marked this as draft, I don't really want to use setters - ideally I would prefer to use kwargs on the constructor but I'm nervous of another fiasco like we had earlier this week.
Re properties not being private, isn't that point behind setters and getters?
There was a problem hiding this comment.
I mean, not **kwargs, but just
def __init__(
self,
path: Path | str | None = None,
wildcard_wrap=default_parser_config.wildcard_wrap,
*,
root_map: RootMap | None = None,
sort_wildcards: bool = True,
deduplicate_wildcards: bool = True,
) -> None:Since they're after the kwarg-only star, there's no chance of a downstream user accidentally passing them wrong, and the defaults would remain the current ones.
As for setters, sure, but then you'd do
@property
def sort_wildcards(self) -> bool:
return self._sort_wildcards
@sort_wildcards.setter
def sort_wildcards(self, value) -> None:
self._sort_wildcards = bool(value)
- but better yet just not mark these as private since they are okay to fiddle with at runtime (unlike
_pathand_cacheetc., for which the class holds some internal assumptions (e.g._pathand_treeand_root_mapare tied together).
| def set_sort_wildcards(self, value: bool): | ||
| """ | ||
| Set whether wildcard values should be sorted. | ||
| """ | ||
| self._sort_wildcards = value | ||
|
|
||
| def set_dedup_wildcards(self, value: bool): | ||
| """ | ||
| Set whether wildcard values should be deduplicated. | ||
| """ | ||
| self._dedup_wildcards = value |
There was a problem hiding this comment.
I mean, not **kwargs, but just
def __init__(
self,
path: Path | str | None = None,
wildcard_wrap=default_parser_config.wildcard_wrap,
*,
root_map: RootMap | None = None,
sort_wildcards: bool = True,
deduplicate_wildcards: bool = True,
) -> None:Since they're after the kwarg-only star, there's no chance of a downstream user accidentally passing them wrong, and the defaults would remain the current ones.
As for setters, sure, but then you'd do
@property
def sort_wildcards(self) -> bool:
return self._sort_wildcards
@sort_wildcards.setter
def sort_wildcards(self, value) -> None:
self._sort_wildcards = bool(value)
- but better yet just not mark these as private since they are okay to fiddle with at runtime (unlike
_pathand_cacheetc., for which the class holds some internal assumptions (e.g._pathand_treeand_root_mapare tied together).
adieyal
force-pushed
the
feature/disable-sorting-and-deduplication
branch
from
eeba0ec
to
e24c1ad
Compare
|
Re - kwarg Suppose we push this release. Then modify the extension to allow for toggling deduplication and sorting. If someone updates their extension but their dynamicprompts doesn't update, then the code will break since the signature doesn't accept these new parameters. I think we should wait to update the extension until adieyal/sd-dynamic-prompts#449 is merged before updating the extension. |
To maintain backwards compatibility, by default wildcards are sorted and deduplicated but this functionality can be disabled.