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

Suggest a GitHub Connection #32

Merged
merged 9 commits into from Oct 30, 2023
Merged

Suggest a GitHub Connection #32

merged 9 commits into from Oct 30, 2023

Conversation

dariagrudzien
Copy link
Collaborator

@dariagrudzien dariagrudzien commented Sep 26, 2023

I broke the event loop 💔

When running the new test I get:

tests/test_lib_intro.py:49:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
juniorguru_chick/lib/intro.py:105: in generate_intro_message
    view = ui.View(ui.Button(emoji='📖',
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <View timeout=180.0 children=2>, timeout = 180.0, disable_on_timeout = False
items = (<Button style=<ButtonStyle.link: 5> url='https://junior.guru/handbook/' disabled=False label='Příručka' emoji=<Partia...nior.guru/courses/' disabled=False label='Kurzy' emoji=<PartialEmoji animated=False name='🧑\u200d🏫' id=None> row=None>)

    def __init__(
        self,
        *items: Item,
        timeout: float | None = 180.0,
        disable_on_timeout: bool = False,
    ):
        self.timeout = timeout
        self.disable_on_timeout = disable_on_timeout
        self.children: list[Item] = []
        for func in self.__view_children_items__:
            item: Item = func.__discord_ui_model_type__(
                **func.__discord_ui_model_kwargs__
            )
            item.callback = partial(func, self, item)
            item._view = self
            setattr(self, func.__name__, item)
            self.children.append(item)

        self.__weights = _ViewWeights(self.children)
        for item in items:
            self.add_item(item)

>       loop = asyncio.get_running_loop()
E       RuntimeError: no running event loop

../../../Library/Caches/pypoetry/virtualenvs/juniorguru-chick-woDL3_j3-py3.11/lib/python3.11/site-packages/discord/ui/view.py:183: RuntimeError

I have a feeling this is the line that causes it: https://github.com/juniorguru/juniorguru-chick/blob/daria/github-connections/juniorguru_chick/bot.py#L85 but not really sure why?

@honzajavorek
Copy link
Member

I can't see anything wrong in the code. I'll have to try it out.

@honzajavorek
Copy link
Member

So it seems just instantiating ui.View() touches the asyncio event loop 🤯 That means one cannot instantiate the object outside of event loop, which is what happens when you test the function. This would bite me too! I'm quite surprised they do this. It's quite annoying as that means the generate_intro_message() isn't really that pure and testable as I thought 😢

@dariagrudzien
Copy link
Collaborator Author

Ah ha! Interesting. I'll refactor tomorrow and try to fix it.

@dariagrudzien
Copy link
Collaborator Author

@honzajavorek Would you mind writing a correct text snippet when you have a moment? No rush.

@honzajavorek
Copy link
Member

Hotfix here #33

Would you mind writing a correct text snippet when you have a moment? No rush.

Sure, putting to my todo.

@dariagrudzien
Copy link
Collaborator Author

@honzajavorek This feature is ready to deploy, needs correct copy.

@honzajavorek
Copy link
Member

Message:

Vidím, že máš **profil na GitHubu**. Když si GitHub propojíš s Discordem, bude tvůj profil viditelnější. Do budoucna navíc chystáme pro lidi s propojeným GitHub profilem spoustu vychytávek <a:yayfrog:976193164471853097>

1. Jdi do [nastavení](https://discord.com/channels/@me)
2. Klikni na Propojení (_Connections_).
3. Přidej GitHub.

@honzajavorek
Copy link
Member

For start, I'd do it like this. If there's no mention of GitHub in the intro, then the message looks like this:

Screenshot 2023-10-04 at 12 21 03

If there's a link to GitHub in the intro, then the message should be:

Dík, že se představuješ! Když o tobě víme víc, můžeme ti líp radit <:meowthumbsup:842730599906279494> 

+ Vidím, že máš **profil na GitHubu**. Když si GitHub propojíš s Discordem, bude tvůj profil viditelnější. Do budoucna navíc chystáme pro lidi s propojeným GitHub profilem spoustu vychytávek <a:yayfrog:976193164471853097>
+ 
+ 1. Jdi do [nastavení](https://discord.com/channels/@me)
+ 2. Klikni na Propojení (_Connections_).
+ 3. Přidej GitHub.

Představení můžeš kdyžtak doplnit či změnit přes tři tečky a „Upravit zprávu“ 📝
...

I may fiddle with it later for better structuring or readability, but I wouldn't bother you with this now.

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.

cat-highfive

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

@honzajavorek I think that I've got it. Is this how you imagined it? Or should the GH version skip the tips at the end to make the message shorter?

image
image

@honzajavorek
Copy link
Member

I think this is awesome. It is indeed rather long message, but let's not fiddle with that. I'll merge this so that we have the feature in place. I'm curious how many people will trigger it, how they'll react to it, and then we can make changes based on observations.

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.

I found a few very small things. I wrote the comments so you can learn or argue, but I'll accept my own suggestions and merge the PR, so we save a few back-and-forths.

juniorguru_chick/bot.py Outdated Show resolved Hide resolved
juniorguru_chick/bot.py Outdated Show resolved Hide resolved
juniorguru_chick/lib/intro.py Show resolved Hide resolved
juniorguru_chick/lib/intro.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@honzajavorek
Copy link
Member

honzajavorek commented Oct 30, 2023

Ouch 😂 So the dedent wasn't unused 😂

ackward-look-monkey

@honzajavorek
Copy link
Member

🎉 Shipping! Unrelated Garfield attached.

garfield-coffee

@honzajavorek honzajavorek merged commit 8c000de into main Oct 30, 2023
2 checks passed
@honzajavorek honzajavorek deleted the daria/github-connections branch October 30, 2023 12:10
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