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

Daria/thread names #27

Merged
merged 5 commits into from Sep 11, 2023
Merged

Daria/thread names #27

merged 5 commits into from Sep 11, 2023

Conversation

dariagrudzien
Copy link
Collaborator

Welp! I got stuck trying to run a basic test o.O

Please disregard the actual meat of the thing. This is my first time with Pytest and parameters, and I'm surprised that I don't see the new test in the report. I checked the pyproject.toml for config and I don't understand why this wouldn't run.

Also for fun when I started at least I was seeing run-last-failure: 4 known failures not in selected tests. Now in pycache directories I see files with the .pyc extension hinting at some assertion errors. When I try to run the specific test file, Pytest doesn't collect any items.

What am I missing?

╰─$ poetry run pytest --verbose
======================================================================================= test session starts ========================================================================================
platform darwin -- Python 3.11.4, pytest-7.4.0, pluggy-1.0.0 -- /Users/d/Library/Caches/pypoetry/virtualenvs/juniorguru-chick-woDL3_j3-py3.11/bin/python
cachedir: .pytest_cache
rootdir: /Users/d/dev/juniorguru/juniorguru-chick
configfile: pyproject.toml
testpaths: .
collected 3 items
run-last-failure: no previously failed tests, not deselecting items.

tests/test_lib_intro.py::test_choose_intro_emojis PASSED                                                                                                                                     [ 33%]
tests/test_lib_intro.py::test_choose_intro_emoji_edge_cases[z\xe1kladn\xed struktury v pythonu a C# v ITnetwork-<:csharp:842666113230045224>] PASSED                                         [ 66%]
tests/test_lib_intro.py::test_choose_intro_emoji_edge_cases[L\xe1k\xe1 m\u011b C++ a C#-<:cpp:842666129071931433>] PASSED                                                                    [100%]

========================================================================================= warnings summary =========================================================================================
../../../Library/Caches/pypoetry/virtualenvs/juniorguru-chick-woDL3_j3-py3.11/lib/python3.11/site-packages/discord/player.py:28
  /Users/d/Library/Caches/pypoetry/virtualenvs/juniorguru-chick-woDL3_j3-py3.11/lib/python3.11/site-packages/discord/player.py:28: DeprecationWarning: 'audioop' is deprecated and slated for removal in Python 3.13
    import audioop

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================================================================== 3 passed, 1 warning in 0.10s ===================================================================================
╭─

@honzajavorek
Copy link
Member

Quick response, as I'm in a forest: The test function must start with "test_" prefix. I'm sorry! It's not obvious, it's a magic pytest convention. The same way xUnit prescribes method names.

@dariagrudzien
Copy link
Collaborator Author

@honzajavorek Thank you! Ofc I missed that.

@honzajavorek
Copy link
Member

honzajavorek commented Aug 14, 2023 via email

tests/test_lib_thread_names.py Outdated Show resolved Hide resolved
1. Check if the message includes text in brackets, if yes:
a. Remove brackets
b. Remove spaces
c. Check if the message includes text between colons
Copy link
Member

Choose a reason for hiding this comment

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

I think there will never be text between colons just like that. User types it there like :css:, but Discord translates it into either:

  • Custom emoji, and it becomes <:css:842343369618751519>
  • Unicode emoji, and it becomes 🐍

For simplicity, I'd keep them as they are. Treat them as if they're any other words, and if that becomes a problem, let's fix that in the future. In such case, I think it should be enough to:

  • See if the beginning (leading whitespace not breaking the algorithm) of the message has a match for [, followed by multiple "anything else than ]", followed by ].
  • If yes, take the "anything else than ]", split it by commas, strip trailing white space.
  • Join the resulting list back to a string and turn it into a thread name as you like.
  • Keep the original message as is, we're just naming threads.
  • If there are no brackets at the beginning, name the thread by the current weekday.

Potentially useful links:

Also, the dispute whether to add walrus operator to Python or not did cause Guido to step down as a BDFL, but in the end we have it! Checking results of re.match and re.search can be one line shorter now: if match := re.match(...): Ofc, I understand it's not for everyone, so up to you. (Just for those brave enough to roll on the beach with walruses, muhaha).

juniorguru_chick/lib/threads.py Show resolved Hide resolved
tests/test_lib_thread_names.py Outdated Show resolved Hide resolved
@dariagrudzien
Copy link
Collaborator Author

dariagrudzien commented Aug 16, 2023

@honzajavorek Thank you for the great tips!
One thing for future—my intention here wasn't to get a review of the code, just to get unblocked (Please disregard the actual meat of the thing.). This is to avoid you wasting time in case I throw something out. If this should happen again and I'd like to show you work in progress which in my mind is not ready for review, how should I explain that?

OMG it's so awesome to learn from you again!
image

@honzajavorek
Copy link
Member

@dariagrudzien You did well, I understood perfectly. Initially I did pass over the meat of the thing, but later I found myself having a bit of time and voluntarily decided I might read the rest and drop a few comments in case I see opportunity to help. Hoping that those won't be taken as unsolicited advice 😅

@dariagrudzien
Copy link
Collaborator Author

@honzajavorek As long as everyone wins, we're good!

@dariagrudzien
Copy link
Collaborator Author

I decided to take a step back and try TDD. Added passing & failing tests.

@dariagrudzien dariagrudzien force-pushed the daria/thread-names branch 3 times, most recently from 7ddc278 to 3fb24c6 Compare September 7, 2023 13:26
Copy link
Member

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

As far as the tests pass, I found no blockers! Made a few suggestions, but up to you.

Only now I realized the name is now composed from just the words, but in #10 the idea is more like „Past na Honza Javorek: algoritmy“ instead of „Čtvrteční past na Honza Javorek”. I think we can improve it this way as a next step in a separate PR though.

juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/threads.py Outdated Show resolved Hide resolved
tests/test_lib_thread_names.py Outdated Show resolved Hide resolved
@dariagrudzien dariagrudzien merged commit 2a7a254 into main Sep 11, 2023
2 checks passed
@dariagrudzien dariagrudzien deleted the daria/thread-names branch September 11, 2023 08:47
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.

None yet

2 participants