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

Jump to bottom

Send USGS Data to Kafka #7

Merged
merged 17 commits into from
Jun 26, 2024
Merged

Send USGS Data to Kafka #7

merged 17 commits into from
Jun 26, 2024

Conversation

Fireye04
Copy link
Member

No description provided.

@Fireye04 Fireye04 requested a review from afausti May 23, 2024 00:02
afausti
afausti requested changes May 23, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment

Choose a reason for hiding this comment

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

Good start! made some suggestions to move forward.

test.json Outdated Show resolved Hide resolved
test.json Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
@Fireye04
Copy link
Member Author

In retrospect, I might've overthought the problem a bit lmao. Let me know what you think!

@Fireye04 Fireye04 requested a review from afausti May 24, 2024 20:27
afausti
afausti requested changes May 28, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment

Choose a reason for hiding this comment

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

Let's start by designing the classes you need, and how they relate. I've suggested a link where you can read about SOLID then we can get together and discuss your design.

src/sasquatchbackpack/schemas/usgs.avsc Outdated Show resolved Hide resolved
src/sasquatchbackpack/schemas/usgs.avsc Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 requested a review from afausti May 30, 2024 23:06
@afausti
Copy link
Member

afausti commented May 31, 2024

I added a commit to modernize this package. You’ll have to recreate your Python environment to use Python 3.12, run make update, and then tox -e lint will report the linting errors, so you can have fun fixing those.

@afausti @Fireye04
Modernize package
8a5f7d3 
- Use ruff for linting
- Update tox.ini and pyproject.toml configurations
- Update .pre-commit-config.yaml
- Update Makefile
- Update dependencies, use requirements/dev.in and requirements/main.in instead
- Update github actions

make udpdate
afausti
afausti requested changes Jun 4, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is looking much better, I think next step is to implement the post() method in the BackpackDispatcher class.
Great job getting the linter passing, hope it was instructive to get over the errors and fix them.
See my comment regarding the deprecation of datetime.utcnow() in Python 3.12.

src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/schemas/usgs.avsc Outdated Show resolved Hide resolved
src/sasquatchbackpack/scripts/usgs.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 requested a review from afausti June 5, 2024 21:46
@afausti
Copy link
Member

afausti commented Jun 5, 2024 •
edited
Loading

I noticed a few inconsistencies between the docstrings and code.
A good way to flag those is using the Sphinx automodapi directive to document the Python API.
I added a commit to reorganize the docs and add a Python API reference section under the Development page.
You can run tox -e docs now and have fun fixing the docstrings.

@afausti @Fireye04
Reorganize docs
61bc443 
- Move CLI docs to the user guide section
- Remove API for now
- Add Python API reference to the Development section
- Remove non-public APIs from the published documentation
- Fix Docstrings
afausti
afausti requested changes Jun 7, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment

Choose a reason for hiding this comment

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

I reviewed sasquatch.py and made some comments regarding error handling and still some issues with docstrings.
Let's iterate on that first, let me know if you have any question.

src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sasquatch.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 requested a review from afausti June 10, 2024 21:53
afausti
afausti requested changes Jun 11, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment

Choose a reason for hiding this comment

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

I've reviewed sources.py let's iterate on that.
I'll add a review to cli.py soon.

src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/sources.py Outdated Show resolved Hide resolved
@afausti afausti self-requested a review June 25, 2024 17:28
afausti
afausti requested changes Jun 25, 2024
View reviewed changes
Copy link
Member

@afausti afausti left a comment

Choose a reason for hiding this comment

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

I've made a few suggestions to improve code readability and avoid interactive prompts in the CLI.

src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
src/sasquatchbackpack/cli.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 requested a review from afausti June 25, 2024 23:45
@Fireye04 Fireye04 merged commit 67193e8 into main Jun 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afausti afausti Awaiting requested review from afausti

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Fireye04 @afausti