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

Cleanup in concurrency module and typing #1173

Merged
merged 2 commits into from
Apr 13, 2023
Merged

Conversation

erlendvollset
Copy link
Collaborator

  • Merge MainThreadExecutor and ExtendedMainThreadExecutor
  • Get rid of some redundant type-ignores

Description

Please describe the change you have made.

Checklist:

  • Tests added/updated.
  • Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
    If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.
  • Changelog updated in CHANGELOG.md.
  • Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

@erlendvollset erlendvollset requested review from a team as code owners March 20, 2023 11:19
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #1173 (75ff5ad) into master (16542ab) will decrease coverage by 0.12%.
The diff coverage is 90.90%.

❗ Current head 75ff5ad differs from pull request most recent head 1fd76e0. Consider uploading reports for the commit 1fd76e0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
- Coverage   90.14%   90.02%   -0.12%     
==========================================
  Files          83       83              
  Lines        9743     9709      -34     
==========================================
- Hits         8783     8741      -42     
- Misses        960      968       +8     
Impacted Files Coverage Δ
cognite/client/utils/_concurrency.py 90.32% <90.00%> (+5.38%) ⬆️
cognite/client/_api/files.py 97.46% <100.00%> (-0.02%) ⬇️

... and 29 files with indirect coverage changes

@haakonvt
Copy link
Contributor

This will raise when passed a function with a named argument priority though. Probably not a big deal?

Ref.:

if "priority" in inspect.signature(fn).parameters:
    raise TypeError(f"Given function {fn} cannot accept reserved parameter name `priority`")

@erlendvollset
Copy link
Collaborator Author

This will raise when passed a function with a named argument priority though. Probably not a big deal?

Ref.:

if "priority" in inspect.signature(fn).parameters:

    raise TypeError(f"Given function {fn} cannot accept reserved parameter name `priority`")

That's fine.

@haakonvt
Copy link
Contributor

I think we should make it easier for SDK developers on 3.9 and 3.10 to contribute without forcing them to use 3.8 to avoid mypy raising issues upon committing.

On this branch with 3.9 and a fresh poetry install, I get the following issues:

Screenshot 2023-03-22 at 12 34 44

...same with 3.10:

Screenshot 2023-03-22 at 12 32 45

...btw, I see that I've introduced this on master (only accepted by mypy with 3.8):

Screenshot 2023-03-22 at 12 29 18

@haakonvt
Copy link
Contributor

Just linking issue here: #1168

@erlendvollset
Copy link
Collaborator Author

I also think it's fine to require contributors to use python 3.8. But let's deal with this in another PR.

Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

Sounds reasonable 😉

@erlendvollset erlendvollset force-pushed the cleanup-concurrency branch 2 times, most recently from 0f3148c to 75ff5ad Compare March 28, 2023 11:05
@erlendvollset erlendvollset enabled auto-merge (squash) April 13, 2023 10:31
@erlendvollset erlendvollset merged commit c314490 into master Apr 13, 2023
@erlendvollset erlendvollset deleted the cleanup-concurrency branch April 13, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants