adding warning when requests_futures isnt installed #1863
Conversation
|
@adehad / @studioj this one looks good to me as-is, but I wonder if the constructor would be a better place for this warning considering it wouldn't be visible until the code attempts to execute the |
There was a problem hiding this comment.
Thanks for the initial review @dimitarOnGithub and for the PR @RichieCahill. I think we should tackle this in a more obvious way if possible, I've added some suggestions in the review
jira/client.py
Outdated
| try: | ||
| from requests_futures.sessions import FuturesSession | ||
|
|
||
| options["async_class"] = FuturesSession | ||
| except ImportError: | ||
| msg = ( | ||
| "async option requires requests-futures to be installed. " | ||
| "falling back to synchronous implementation." | ||
| ) | ||
| warnings.warn(msg) | ||
| pass |
There was a problem hiding this comment.
My preference would be to:
- do the import check outside the scope of this class, then either set
FutureSessionasNoneif it is not imported or have a boolean variable, eg._has_async_support. - This warning can be emitted in init as you have done if the async option is passed incorrectly.
Furthermore I would augment this warning message with a suggestion of how to install this extra requirement, as we do have this as an extra, it may be appropriate to say,this can be installed using the 'async' extra, e.g. pip install jira[async] - I would be tempted to modify the option itself to be False and then no further code changes need to be made in the dependency functions.
- Code that needs the async may check for the boolean or that the imported class is not None.
There was a problem hiding this comment.
ive updated the code based on your suggestion. Im unclear on what 3 means.
| if ( | ||
| async_class is not None | ||
| and not is_last | ||
| and (total is not None and len(items) < total) | ||
| ): | ||
| async_fetches = [] | ||
| future_session = async_class( | ||
| session=self._session, max_workers=async_workers | ||
| session=self._session, | ||
| max_workers=self._options["async_workers"], |
There was a problem hiding this comment.
Just to preserve the original code I would suggest keep this as
| max_workers=self._options["async_workers"], | |
| max_workers=self._options.get("async_workers"), |
There was a problem hiding this comment.
what value dose preserve the original code add in this case?
|
should we insted of warning if async=true and requests_futures isnt installed just raise an exception |
|
Label error. Requires exactly 1 of: bug, enhancement, major, minor, patch, skip-changelog. Found: |
|
@adehad please take a look at this pr i have updated parts of it |
This is a super small pr. I dont think it requires any new tests because of its nature.
Please let me know if I did anything wrong.