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

#126: fix MakeNote.key_to_log not being updated. #129

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

perrygoy
Copy link
Member

In MakeNote.as_, we forget to update key_to_log. This caused the logs to read things like:

Guido jots something down under <None>.
Guido jots something down under <None>.
Guido jots something down under <None>.

Now!

Guido jots something down under "my birthday".
Guido jots something down under "gift ideas".
Guido jots something down under "friends to invite".

@perrygoy perrygoy requested a review from a team February 16, 2024 16:16
@@ -384,6 +384,7 @@ def test_key_value_set(self) -> None:

assert mn.question == test_question
assert mn.key == test_key
assert mn.key_to_log == f"'{test_key}'"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the only test we need, as our typehints try to tell users to only pass strs to MakeNote.as_. If i add a test for passing a non-string to as_, MyPy complains about it. :P

This test shows that using .as_ updates key_to_log at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a test to prove MakeNote(question, key) displays the right thing, but I think if we're going to eventually make key_to_log into a property, we can address the problem there.

@@ -56,6 +57,7 @@ def of_the(cls, question: T_Q) -> Self:
def as_(self, key: str) -> Self:
"""Set the key to use to recall this noted value."""
self.key = key
self.key_to_log = represent_prop(key)
Copy link
Contributor

@bandophahita bandophahita Feb 20, 2024

Choose a reason for hiding this comment

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

This make sense, but I'm starting to wonder if we should consider a wholesale update to any action similar to this; I think we ought to make key_to_log a property. (in a future PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good call, and yeah let's make that happen soon.

Copy link
Contributor

@bandophahita bandophahita left a comment

Choose a reason for hiding this comment

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

Looks good.

@perrygoy perrygoy merged commit ed809bf into trunk Feb 20, 2024
23 checks passed
@perrygoy perrygoy deleted the 126/fix-make-note-log branch February 20, 2024 22:08
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.

2 participants