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 ProfileNetCDFWriter to include the dba to ngdac conversions #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Bobfrat
Copy link
Collaborator

@Bobfrat Bobfrat commented Jun 27, 2018

@kerfoot I'm going to be importing this package into the upload tools code rather than running it from the command line, so this refactor will make it easier to use by putting most of the heavy lifting from dba_to_ngdac_profile_nc.py into the ProfileNetCDFWriter class.

If you're ok with this, I can do the same with the other scripts: dba_to_profile_nc.py and dba_to_trajectory_nc.py

@Bobfrat Bobfrat requested a review from kerfoot June 27, 2018 14:49
@kerfoot
Copy link
Owner

kerfoot commented Jun 27, 2018

@Bobfrat Would it be a major pain to subclass ProfileNetCDFWriter to something like DbaProfileNetCDFWriter and just add this method? I really would like to keep the ProfileNetCDFWriter source-file-agnostic so that I/we don't have to keep adding methods to the class.

Thoughts?

@Bobfrat
Copy link
Collaborator Author

Bobfrat commented Jun 27, 2018

yeah thats not a problem at all, I'll update this PR

@Bobfrat
Copy link
Collaborator Author

Bobfrat commented Aug 2, 2018

@kerfoot just made that update, can you have a look?

Copy link
Owner

@kerfoot kerfoot left a comment

Choose a reason for hiding this comment

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

@Bobfrat

  1. The ProfileNetCDFWriter class has a self._logger logger. Can you change all of the logging calls in the method to use the ProfileNetCDFWriter class logger?
  2. I've made some changes to the TrajectoryNetCDFWriter class for the navy work. Don't think there are conflicts but I'd like to push the changes before merging and test. Does that work for you?

@Bobfrat
Copy link
Collaborator Author

Bobfrat commented Aug 3, 2018

@kerfoot sounds good on both comments

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.

2 participants