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.

Sign up for GitHub

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

adding warning when requests_futures isnt installed #1863

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

adding warning when requests_futures isnt installed #1863

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74f6490
adding warning when requests_futures isnt installed
RichieCahill May 25, 2024
e19640d
updated afters dimitarOnGithub input
RichieCahill Jul 1, 2024
4fe96c9
updated with adehad suggestions
RichieCahill Jul 10, 2024
797ec7c
fixed bug
RichieCahill Jul 11, 2024
34ae65e
fixed bug
RichieCahill Jul 11, 2024
8c0af70
Merge branch 'main' into feature/adding-warning-when-requests_futures…
RichieCahill Jul 11, 2024
66f9d55
Merge branch 'main' into feature/adding-warning-when-requests_futures…
RichieCahill Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions jira/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,22 @@ def __init__(

if server:
options["server"] = server

options["async_class"] = None
if async_:
if self._options["async"]:
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
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to:

  1. do the import check outside the scope of this class, then either set FutureSession as None if it is not imported or have a boolean variable, eg. _has_async_support.
  2. 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]
  3. 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.
  4. Code that needs the async may check for the boolean or that the imported class is not None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ive updated the code based on your suggestion. Im unclear on what 3 means.


options["async"] = async_
options["async_workers"] = async_workers

Expand Down Expand Up @@ -790,16 +805,6 @@ def _fetch_pages(
Returns:
ResultList
"""
async_workers = None
async_class = None
if self._options["async"]:
try:
from requests_futures.sessions import FuturesSession

async_class = FuturesSession
except ImportError:
pass
async_workers = self._options.get("async_workers")

def json_params() -> dict[str, Any]:
# passing through json.dumps and json.loads ensures json
Expand Down Expand Up @@ -847,14 +852,16 @@ def json_params() -> dict[str, Any]:
page_size,
)
page_start = (startAt or start_at_from_response or 0) + page_size
async_class = self._options["async_class"]
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"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to preserve the original code I would suggest keep this as

Suggested change
max_workers=self._options["async_workers"],
max_workers=self._options.get("async_workers"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what value dose preserve the original code add in this case?

)
for start_index in range(page_start, total, page_size):
page_params = json_params()
Expand Down
Loading