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

[Feature] Improve handling of local mode & docs regarding file handling #3114

Closed
Bibo-Joshi opened this issue Jun 22, 2022 · 5 comments · Fixed by #3154
Closed

[Feature] Improve handling of local mode & docs regarding file handling #3114

Bibo-Joshi opened this issue Jun 22, 2022 · 5 comments · Fixed by #3154

Comments

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jun 22, 2022

I suggest to add a flag local_api_server/local_mode/similar naming to the Bot class which can be used to tell ptb that the bot is running against a bot api server in --local mode. Currently the benefit of that would be that we could accept file paths in all cases. In local mode, we pass the URI, otherwise we load the contents in binary mode.
If the local mode has more benefits in the future, we could leveral this setting.

What do the other team members think about this?

Additionally, we should unify the documentation on how files are handled.
Some of the classes/methods that accept file input have a note like here, but others don't. Also that note is outdated, as it doesn't explain when a pathlib.Path can be passed. It should also mention that we don't support open(file, 'r') but only open(file, 'rb'). We should either have a unified note that we add everywhere or move this snippets section to a standalone page, extend it a bit and point to it from the docs.
I'm slightly in favor of the latter.

To avoid repitition of Note: … strings in the docs, we can .. include:: them - see sphinx-doc/sphinx/issues/9939

@Poolitzer
Copy link
Member

Poolitzer commented Jun 25, 2022

What do the other team members think about this?

I think it is a small improvement, and I don't think that much more benefits will come to the local API, especially not some which would change further how our library handles stuff. On the other hand, its a big change (and allows the local server to be used as intended), so I think its a good one.

I think a note for every input handling a file is the best solution, and since that would take quite some additional screen time, linking to a second page where it is explained in detail how file handling works in PTB is good.

@Bibo-Joshi
Copy link
Member Author

I think it is a small improvement, and I don't think that much more benefits will come to the local API, especially not some which would change further how our library handles stuff. On the other hand, its a big change (and allows the local server to be used as intended), so I think its a good one.

I'm a big confused here 😄 "I think it is a small improvement" vs "its a big change". Do you mean that the added value is small while the code/compatiblity change is big?

@Poolitzer
Copy link
Member

Sorry, mistyped. Small change, big improvement.

@harshil21
Copy link
Member

harshil21 commented Jul 6, 2022

About the sphinx issue linked.. I found an alternative - We could use rst Substitutions in such places too. Maybe not as effective as an include, but it would reduce repetition in _bot.py specially (the timeout + api_kwargs param). I tested the below and it works:

_bot.py:

class Bot(TGObject):
    """....
        .. |read_timeout| replace:: Value to pass to ...
    """

    async def get_me(...):
    """
    Keyword Args:
        read_timeout (:obj:`....`): |read_timeout|
        ...
    """

If you all agree this is effective enough for the time being, I can open a good first issue for this.

@Bibo-Joshi
Copy link
Member Author

Interesting! covering the name & type part probably doesn't work? if not: TBH I feel like it would be worth to first try and PR to sphinx about sphinx-doc/sphinx#9939. OTOH, switching from a |replace| solution to a .. include:: solution if sphinx-doc/sphinx#9939 ever gets resolved will probably be rather easy 🤔 So I'm probably okay with both :D

@Bibo-Joshi Bibo-Joshi mentioned this issue Jul 13, 2022
5 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants