8000 enhance(s3): add support in decorator, change module location by lucasrodes · Pull Request #47 · owid/owid-datautils-py · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Nov 1, 2023. It is now read-only.

enhance(s3): add suppor 10000 t in decorator, change module location #47

Merged
merged 14 commits into from
Oct 17, 2022
Merged

enhance(s3): add support in decorator, change module location #47

merged 14 commits into from
Oct 17, 2022

Conversation

lucasrodes
Copy link
Member
@lucasrodes lucasrodes commented Oct 16, 2022 •
edited
Loading
  • Add support for s3 file downloading to decorator owid.datautils.decorators.enable_file_download
  • Moved owid.datautils.io.s3 --> owid.datautils.s3. Why? s3 does not need to be something strictly io-related. Also, it might be a good practice to avoid circular imports between owid.datautils.io and owid.datautils.decorators:
    • owid.datautils.io.json and owid.datautils.io.archive have imports from owid.datautils.decorators
    • owid.datautils.decorators has imports from owid.datautils.s3

@lucasrodes lucasrodes added the enhancement New feature or request label Oct 16, 2022
@lucasrodes lucasrodes changed the title enhance(s3) enhance(s3): add support in decorator, change module location Oct 16, 2022
@lucasrodes lucasrodes changed the base branch from enhance/json-files to main October 16, 2022 22:13
@lucasrodes lucasrodes changed the base branch from main to enhance/json-files October 16, 2022 22:14
@lucasrodes lucasrodes changed the base branch from enhance/json-files to main October 16, 2022 22:17
@lucasrodes lucasrodes changed the base branch from main to enhance/json-files October 16, 2022 22:17
pabloarosado
pabloarosado approved these changes Oct 17, 2022
View reviewed changes
Copy link
Collaborator
@pabloarosado pabloarosado left a comment

Choose a reason for hiding this comment

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

I think these changes will break other repos (energy-data, co2-data, poverty-data, and possibly etl, wherever datautils.io.s3 was used). Do you want to take care of refactoring accordingly?

@lucasrodes
Copy link
Member Author

@pabloarosado answering your points from #45 (review) that concern this PR.

I think your initial concern was to avoid having interdependencies among modules. But I don't see how this has changed: Now io.archive imports decorators, and decorators imports web. In any case, I don't think we should worry about this.

That's correct. But if we keep it as it is, i.e. io.s3 or move web to io.web we would have that:

io imports several functions from its submodules, including io.archive; io.archive imports decorators; decorators imports io.s3 and io.web.

I can see how this is getting into circular imports.

@lucasrodes
Copy link
Member Author

@pabloarosado happy to refactor ー as long as we don't bump de version here and update the version dependency in etl it should be fine, right?

@pabloarosado
Copy link
Collaborator

@pabloarosado happy to refactor ー as long as we don't bump de version here and update the version dependency in etl it should be fine, right?

I think bumping version here is irrelevant. I think all repos using datautils are specifying the version to install, so there's no danger for now.

@lucasrodes lucasrodes changed the base branch from enhance/json-files to main October 17, 2022 10:34
@codecov
Copy link
codecov bot commented Oct 17, 2022

Codecov Report

Merging #47 (927705d) into main (861235d) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   87.45%   87.61%   +0.15%     
==========================================
  Files          10       10              
  Lines         550      557       +7     
==========================================
+ Hits          481      488       +7     
  Misses         69       69
Impacted Files Coverage Δ
owid/datautils/s3.py 62.50% <ø> (ø)
owid/datautils/decorators.py 100.00% <100.00%> (ø)

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

This was referenced Oct 17, 2022
Closed
Closed
Draft
@lucasrodes lucasrodes mentioned this pull request Oct 17, 2022
Closed
@lucasrodes lucasrodes merged commit 190e42a into main Oct 17, 2022
@lucasrodes lucasrodes deleted the enhance/s3 branch October 17, 2022 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pabloarosado pabloarosado pabloarosado approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucasrodes @pabloarosado
0