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

refactor: pkg_resources -> importlib.resources #24578

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

cwegener
Copy link
Contributor

@cwegener cwegener commented Jul 3, 2023

(Part 1 of 2)

setuptools' pkg_resources is in the process of being deprecated in favor
of importlib.resources and importlib.metadata (see also PR #24514)

importlib.resources has been available in the standard library since
Python 3.7

This change follows the importlib_resources migration guide and does not
perform any additional refactoring.

The last component that is using pkg_resources is
'superset.config.BASE_DIR'

'superset.config.BASE_DIR' is mainly used as the default file upload
directory in Flask and therefore requires special attention as this
directory needs to be writable and needs to exist for the full lifetime
of the Flask application.

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #24578 (6ccc37e) into master (226c7f8) will increase coverage by 0.00%.
The diff coverage is 26.66%.

❗ Current head 6ccc37e differs from pull request most recent head f864d01. Consider uploading reports for the commit f864d01 to get more accurate results

@@           Coverage Diff           @@
##           master   #24578   +/-   ##
=======================================
  Coverage   69.08%   69.08%           
=======================================
  Files        1906     1906           
  Lines       74168    74169    +1     
  Branches     8164     8164           
=======================================
+ Hits        51239    51240    +1     
  Misses      20807    20807           
  Partials     2122     2122           
Flag Coverage Δ
hive 53.93% <26.66%> (+<0.01%) ⬆️
mysql 79.40% <26.66%> (+<0.01%) ⬆️
postgres 79.48% <26.66%> (+<0.01%) ⬆️
presto 53.83% <26.66%> (+<0.01%) ⬆️
python 83.48% <26.66%> (+<0.01%) ⬆️
sqlite 78.04% <26.66%> (+<0.01%) ⬆️
unit 54.69% <26.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/examples/utils.py 0.00% <0.00%> (ø)
superset/views/base.py 73.33% <25.00%> (ø)
superset/config.py 92.06% <100.00%> (+0.02%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

(Part 1 of 2)

setuptools' pkg_resources is in the process of being deprecated in favor
of importlib.resources and importlib.metadata (see also PR apache#24514)

importlib.resources has been available in the standard library since
Python 3.7

This change follows the importlib_resources migration guide and does not
perform any additional refactoring.

The last component that is using pkg_resources is
'superset.config.BASE_DIR'

'superset.config.BASE_DIR' is mainly used as the default file upload
directory in Flask and therefore requires special attention as this
directory needs to be writable and needs to exist for the full lifetime
of the Flask application.
@cwegener cwegener changed the title refactor:pkg_resources -> importlib.resources(1/2) refactor: pkg_resources -> importlib.resources Jul 3, 2023
@cwegener
Copy link
Contributor Author

cwegener commented Jul 3, 2023

cc @john-bodley this is the next part in the pkg_resources refactor. It's ready for a review.

(The next and final part of the refactor will need to deal with the superset.config.BASE_DIR which is used for file uploads and that one opens up an opportunity to actually improve the file upload location)

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup!

queue.extend(
path_name / child_name
for child_name in resource_listdir("superset", str(path_name))
path_name / str(child_name)
Copy link
Member

Choose a reason for hiding this comment

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

Why is str(...) needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'll double check. I might have been blindly following mypy errors. Now I'm not sure if the mypy errors were even valid errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I did some digging on joinpath().

mypy was correct. The argument to joinpath() is defined to be of StrPath type which is defined as Union[str, PathLike[str]]

So, whilst Traversable.join_path(Traversable) will execute in CPython (likely due to coercion), it breaks the API protocol of joinpath()

)
elif path_name.suffix.lower() in YAML_EXTENSIONS:
elif Path(str(path_name)).suffix.lower() in YAML_EXTENSIONS:
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a straight forward explicit coercion from Traversable -> str -> Path in order then call the Path.suffix() method.

contents[path_name] = (
resource_stream("superset", str(path_name)).read().decode("utf-8")
)
contents[Path(str(path_name))] = (
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a straight forward explicit coercion from Traversable -> str -> Path in order to create the key for the contents typed dict which has a contents: dict[Path, str] type.

@cwegener cwegener requested a review from john-bodley July 5, 2023 21:23
@john-bodley john-bodley merged commit 7081a0e into apache:master Jul 5, 2023
30 of 31 checks passed
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants