Improvements #14
Improvements #14
Conversation
| self._temp_prompt_folder(prompt_name) | ||
| # TODO: This approach is buggy as, if a text is not found, it will end up returning a FolderPrompts object | ||
| return self |
There was a problem hiding this comment.
Modifying the instance itself is a bit dangerous (and not thread-safe, but that's true for this code in general you'd need some locks on the cache for that).
Also, I guess that if the instance is not "reset" then it may affect future calls to this instance which can be surprising.
I guess the better option would be to return a new object instead of modifying this, and keep track of the folder level. The cache should be shared between instances, otherwise you'd always use an empty cache for subfolders.
You could have a class that tracks the cache and the prefix, say CacheAndPrefix (naming is hard).
When creating an instance of FolderPrompts you start from an empty one, and here you return an instance of FolderPrompts passing in a new instance of CacheAndPrefix that shares the same prompts cache object but adds the attribute name to the prefix. All methods of FolderPrompts then should operate referencing this state prefix & cache. In this way you don't have to have mutable state.
wtprompt/core.py
Outdated
| @@ -7,7 +7,6 @@ | |||
| from typing import Optional, Union | |||
|
|
|||
| from pydantic import BaseModel, Field, field_validator | |||
| from sympy.simplify.simplify import product_mul | |||
There was a problem hiding this comment.
I'd git rebase --interactive and specify squash for this on top of the previous one. The commit does not really provide any value by itself so just rewrite the previous one to remove the error.
| prompt_hashes[key] = hashlib.sha256(prompt.encode("utf-8")).hexdigest() | ||
|
|
||
| # Saving report | ||
| outfile = pathlib.Path(outfile).with_suffix('.json') |
There was a problem hiding this comment.
nitpicking: I personally prefer letting the user choose whatever extension they want.
If they want a .json they can just call this with outfile ending with that suffix.
wtprompt/core.py
Outdated
| prompt_text = self._get_prompt_text(prompt_name) | ||
| computed_hash = hashlib.sha256(prompt_text.encode("utf-8")).hexdigest() | ||
| if computed_hash != prompt_hash: | ||
| warnings.showwarning(f"Prompt {prompt_name} has different has!\n" |
wtprompt/core.py
Outdated
|
|
||
| def _get_prompt_text(self, prompt_name: str): | ||
| return self._prompts[prompt_name] | ||
|
|
||
| def get_prompts(self): | ||
| return self._prompts | ||
| def get_prompt(self, prompt_name: str, get_hash: bool=False) -> Union[str, Tuple[str, str]]: |
There was a problem hiding this comment.
IF you want to be fancy with typing you can use typing.overload to specify that if get_hash is True then this always returns a tuple, if get_hash is False it always return a str.
Alternatively you could have a separate get_prompt_with_hash function instead of the get_hash flag, which might be better and simpler:
def get_prompt_with_hash(self, prompt_name: str) -> Tuple[str, str]:
return self._prompts[prompt_name], self._prompt_hashes[prompt_name]
def get_prompt(self, prompt_name: str) -> str:
return self.get_prompt_with_hash(prompt_name)[0]
| try: | ||
| print(base_prompts.nonexistentprompt) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Using pytest.raises is better:
with pytest.raises(ValueError):
print(base_prompts.nonexistentprompt)
If you need to inspect the exception object raised you can also do:
with pytest.raises(ValueError) as some_error:
print(base_prompts.nonexistentprompt)
some_error.value # exception caught
There was a problem hiding this comment.
This commit mentions updating the Readme but also modifies the tests.
Would be better split in 2.
| version = '0.1.1' | ||
| version = '0.1.11' |
There was a problem hiding this comment.
Why the gap? If you want to signal that this is a bigger change than bugfixes just move to 0.2.0 I'd say.
Solves #9