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

New layout #6

Merged
merged 15 commits into from
Feb 15, 2023
Merged

New layout #6

merged 15 commits into from
Feb 15, 2023

Conversation

raybellwaves
Copy link
Contributor

@raybellwaves raybellwaves commented Feb 10, 2023

new layout: moves tests to root folder, rm setup.py and use pyproject.toml
use latest s3fs moto patch from aio-libs/aiobotocore#755 (comment)
added a KeyError(f"{urlpah}")
fixed a mypy complain with https://github.com/hauntsaninja/no_implicit_optional

@tdhopper
Copy link
Collaborator

you don't want to commit pycache directories

.gitignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@blackary blackary left a comment

Choose a reason for hiding this comment

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

Seems like a fine set of changes overall. A few decisions that wouldn't be my favorite:

  • I do like automagic calver, but doing it manually is certainly simpler
  • I'd put the gitignore back
  • I do prefer putting source code in src/ to make it harder to accidentally import the local module, rather than the package, when testing the package

But, none of those seem like a big deal, and there are some significant wins here, so seems fine to me overall.

requirements.txt Outdated
intake
s3fs
msgpack
requests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why we need this, when we don't use it explicitly in the codebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intake/intake#706
if Martin does another intake release we can remove

Choose a reason for hiding this comment

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

🦸

@raybellwaves
Copy link
Contributor Author

thanks for the review gents (@blackary @tdhopper). Going to merge and work on a release GHA

@raybellwaves raybellwaves merged commit f21d8ba into DTN-Public:main Feb 15, 2023
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.

4 participants