Skip to content

Admonition overhaul#4462

Merged
Bibo-Joshi merged 10 commits intomasterfrom
admonition-overhaul
Jan 31, 2025
Merged

Admonition overhaul#4462
Bibo-Joshi merged 10 commits intomasterfrom
admonition-overhaul

Conversation

@harshil21
Copy link
Member

@harshil21 harshil21 commented Sep 8, 2024 •
edited by Bibo-Joshi
Loading

Main changes so far:

  • Uses type hints for introspection instead of parsing docs
  • Closes Fix GameHighScore return in admonition #4414 (I think, need to check lol)
  • Less LoC
  • Adds Application in the admonitions, so far this has only resulted in a "use-in" from all the handlers to add_handler(s).

To do:

  • Many admonitions are missing - investigate why
  • drastically update the test file for this, for easier iteration
  • pre-commit intentionally being failed for now

@harshil21 harshil21 added enhancement ⚙️ documentation affected functionality: documentation labels Sep 8, 2024
@harshil21 harshil21 marked this pull request as draft September 8, 2024 04:43
@harshil21 harshil21 added the WIP label Sep 8, 2024
@harshil21 harshil21 changed the title Admonition overhaul WIP: Admonition overhaul Sep 8, 2024
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed WIP labels Nov 3, 2024
@codecov
Copy link

codecov bot commented Jan 24, 2025 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
6577 1 6576 673
View the top 1 failed tests by shortest run time 
tests.test_bot.TestBotWithRequest::test_send_close_date_default_tz[UTC-ZoneInfo]
Stack Traces | 5.21s run time 
self = <tests.test_bot.TestBotWithRequest object at 0x000001E131D60950>
tz_bot = PytestExtBot[token=690091347:AAFLmR5pAB5Ycpe_mOh7zM4JFBOh0z3T0To]
super_group_id = '-1001279600026'

    async def test_send_close_date_default_tz(self, tz_bot, super_group_id):
        question = "Is this a test?"
        answers = ["Yes", "No", "Maybe"]
        reply_markup = InlineKeyboardMarkup.from_button(
            InlineKeyboardButton(text="text", callback_data="data")
        )
    
        aware_close_date = dtm.datetime.now(tz=tz_bot.defaults.tzinfo) + dtm.timedelta(seconds=5)
        close_date = aware_close_date.replace(tzinfo=None)
    
        msg = await tz_bot.send_poll(  # The timezone returned from this is always converted to UTC
            chat_id=super_group_id,
            question=question,
            options=answers,
            close_date=close_date,
            read_timeout=60,
        )
        msg.poll._unfreeze()
        # Sometimes there can be a few seconds delay, so don't let the test fail due to that-
>       msg.poll.close_date = msg.poll.close_date.astimezone(aware_close_date.tzinfo)
E       AttributeError: 'NoneType' object has no attribute 'astimezone'

tests\test_bot.py:2837: AttributeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 24, 2025

Hi @harshil21 I got around to work a bit on this. You already had the most part covered, I just patched a few things. Regarding your points:

Many admonitions are missing - investigate why

Do you recall a list of things that you were missing? I clicked around thorugh the docs a bit aimlessly and didin't catch anything.

drastically update the test file for this, for easier iteration

Herer I'm also not sure what to do about it. Adding stuff that we find while working on the implementation is so far the most sane thing to do IMO. Trying to cover all cases would basically result in re-implementing the insertion-logic within the test file, I guess 😅

@harshil21
Copy link
Member Author

harshil21 commented Jan 24, 2025

Do you recall a list of things that you were missing? I clicked around thorugh the docs a bit aimlessly and didin't catch anything.

I looked through my notes and didn't find too much other than:

LinkPreviewOptions "use-in" should show a use in InputTextMessageContent

InputTextMessageContent's available in should be moved up

test to check all service messages are included in StatusUpdate.

but I'm pretty sure there were more classes, but I don't remember them.

Trying to cover all cases would basically result in re-implementing the insertion-logic within the test file

I was just thinking that the test file would have a bunch of more randomly selected cases from the docs and just checks if they are present

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 25, 2025 •
edited
Loading

LinkPreviewOptions "use-in" should show a use in InputTextMessageContent

I skipped this one for now since so far we don't link to any arguments of tg classes

InputTextMessageContent's available in should be moved up

seems ok

test to check all service messages are included in StatusUpdate.

I don't quite get this one. Are you referring to ext.filters.StatusUpdate? We have no linking to ext.filters at all so far …

@Bibo-Joshi Bibo-Joshi changed the title WIP: Admonition overhaul Admonition overhaul Jan 26, 2025
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review January 26, 2025 00:15
harshil21
harshil21 commented Jan 31, 2025
View reviewed changes
Copy link
Member Author

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

looked at the docs and it everything seems to be in order. LGTM.

@Bibo-Joshi Bibo-Joshi merged commit d7e063d into master Jan 31, 2025
26 checks passed
@Bibo-Joshi Bibo-Joshi deleted the admonition-overhaul branch January 31, 2025 18:23
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

⚙️ documentation affected functionality: documentation 🔌 enhancement pr description: enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix GameHighScore return in admonition

2 participants

@harshil21 @Bibo-Joshi