-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update docs, remove unused option, small cleanups #50
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against f958eb7 |
buildstockbatch/gcp/gcp.py
Outdated
code later. Also, because that branch has not yet been merged, this will also _not_ do any | ||
refactoring right now to share code with that (to reduce merging complexity later). Instead, code | ||
that's likely to be refactored out will be commented with 'todo: aws-shared'. | ||
Architecture overview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the right level of detail here is, but I felt we were missing a high-level outline of how we're using GCP (the kind that would have been useful when we were first trying to understand the AWS implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍! I think you should add that many of these things are done by the base classes (including sampling, and input files).
I might suggest that a README.md would be better, especially since I think it'd be best to document more holistically (i.e., relationship with docker_base and aws), but I'm fine with this incremental improvement, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note that this logic is split between the two places.
Yeah, a readme might be better if we want to add more detail later. I'm thinking of this section as mainly "Here's how we're using GCP", which is pretty specific to gcp.py, but a more general "Here's how the code is structured" could also be helpful.
buildstockbatch/gcp/gcp.py
Outdated
code later. Also, because that branch has not yet been merged, this will also _not_ do any | ||
refactoring right now to share code with that (to reduce merging complexity later). Instead, code | ||
that's likely to be refactored out will be commented with 'todo: aws-shared'. | ||
Architecture overview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍! I think you should add that many of these things are done by the base classes (including sampling, and input files).
I might suggest that a README.md would be better, especially since I think it'd be best to document more holistically (i.e., relationship with docker_base and aws), but I'm fine with this incremental improvement, at least.
Tested: Did a small test run to check the code changes.