Skip to content

Conversation

@hyfc
Copy link
Contributor

@hyfc hyfc commented Feb 7, 2023

Fixes #3536 which causes false warning message during bot initialization. Now telegram.ext._utils.stack.was_called_by will always resolve paths in frame before comparing them with caller path.

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs and all suitable __all__ s

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Thanks for the quick PR! Nice that you also managed to write a test case for it 👍 I have a few comments about that:

  • Could you replace the os calls by the corresponding pathlib equivalents?
  • I'm wondering if pytest could help with creating temporary directories: https://docs.pytest.org/en/6.2.x/tmpdir.html. that could make it possible to remove the custom cleanup logic. Plus, if the temporary directories/files are not created in the test directory, there no risk of producing left over artifacts ...
  • IISC your test case currently cover only one of the changes lines. Maybe it's possible to cover also the second one?

@harshil21 harshil21 added this to the v20.1 milestone Feb 8, 2023
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! I have some more nitpicking, but nitpicking is really all it is :)

@Bibo-Joshi
Copy link
Member

btw, don't worry about the faild Check Type Completeness check, that apparently is a problem of the workflow …

hyfc and others added 4 commits February 8, 2023 21:19
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
@hyfc
Copy link
Contributor Author

hyfc commented Feb 8, 2023

Looks like adding resolve may cause trouble on Windows: CI log
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: '<string>'

Haven't tested it on Windows, but if it breaks, I'm ok to drop this PR. We can live with a minor warning message:sweat_smile: .

@Bibo-Joshi
Copy link
Member

That's unfortunate :/ I'll try to have a closer look at it soon™, but I agree that the priority on this may not be too high :D

@Bibo-Joshi Bibo-Joshi removed this from the v20.1 milestone Feb 9, 2023
@hyfc hyfc closed this Feb 9, 2023
@hyfc
Copy link
Contributor Author

hyfc commented Feb 9, 2023

Not able to reproduce the failed TestApplication unit tests on Windows 11 with Python 3.9 and Python 3.11.
I suspect it has something to do with the built-in nt._getfinalpathname on some other versions of Windows.

@Bibo-Joshi Bibo-Joshi reopened this Feb 11, 2023
@Bibo-Joshi Bibo-Joshi closed this Feb 11, 2023
@Bibo-Joshi Bibo-Joshi mentioned this pull request Feb 11, 2023
4 tasks
@Bibo-Joshi
Copy link
Member

FYI: I tried around some more in #3552 and arrived at a solution that I'm happy with.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2023
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 bug pr description: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] telegram.ext._utils.stack.was_called_by gives incorrect result on 64-bit machine

3 participants