GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Admonition overhaul by harshil21 · Pull Request #4462 · python-telegram-bot/python-telegram-bot

Conversation

@harshil21

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Bibo-Joshi reacted with rocket emoji
All reactions
  • 🚀 1 reaction
harshil21 added 4 commits August 3, 2024 00:09
@harshil21
Fix available in admonitions
356a58f
@harshil21
Merge branch 'master' into admonition-overhaul
743517c
@harshil21
WIP: Get basic idea working
42103f0
@harshil21
Merge branch 'master' into admonition-overhaul
f47df21
@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
Bibo-Joshi added 4 commits January 24, 2025 16:44
@Bibo-Joshi
merge master
30618c1
@Bibo-Joshi
Apply some fixes and extend tests accordingly
85dd06b
@Bibo-Joshi
Small CSS improvement
57b3015
@Bibo-Joshi
Merge branch 'master' into admonition-overhaul
880c760
@codecov

codecov Bot commented Jan 24, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

❌ 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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Bibo-Joshi

Bibo-Joshi commented Jan 24, 2025

Copy link
Copy Markdown
Member

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 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@harshil21

harshil21 commented Jan 24, 2025

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Bibo-Joshi
Extend available-in for properties
c5c6d44
@Bibo-Joshi

Bibo-Joshi commented Jan 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 …
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Bibo-Joshi
Fix bug in Application Slots
4069faf
@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

@harshil21 harshil21 left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Bibo-Joshi
Bibo-Joshi merged commit d7e063d into master Jan 31, 2025
@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

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@harshil21 @Bibo-Joshi
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.