netcdf2ascii_repo issueshttps://code.arm.gov/giansiracusa/netcdf2ascii/-/issues2018-07-11T16:57:04Zhttps://code.arm.gov/giansiracusa/netcdf2ascii/-/issues/1quick review2018-07-11T16:57:04ZZach Pricequick reviewThis seems like the most logical way to give a review since we're not yet working from pull requests. Nothing game breaking here, so I'm going to try to be super nit picky just for the sake of the process =)
https://code.arm.gov/giansir...This seems like the most logical way to give a review since we're not yet working from pull requests. Nothing game breaking here, so I'm going to try to be super nit picky just for the sake of the process =)
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L634
I'd use the Requests library here just because url encoding and stuff has a lot of sharp edges and corner cases
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L677
This is an organization problem, we need to come up with a better solution as a group but email is a bad error handler
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L733
You can pass `required=True` to these and argparse will handle this nicely, no need to check in the main loop
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L750
`choices=[' ', ',', '-', '\t', 'tab']` will enforce this and show a nice error message
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L754
you could use the Arrow lib accept quasi-fuzzy times, it takes a string and makes guesses will turning it into a timestamp
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L759
you could use a custom Action to mark these as deprecated: https://gist.github.com/alzix/4868d66115a55567d3b43b32742f6f4f
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L771
no need to call this directly, set some required args and argparse will print help when needed
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L53
quite a few libs out there depend on knowing how python's path is calculated (not just what it currently is), so manual path manipulation like this is generally frowned upon. Instead, we should install everything into its own virtual environment
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L69
I believe all of these can be handled by custom actions and etc in argparse.
Might also be a good idea to move the cli processing stuff into a separate module from the actual business logic
https://code.arm.gov/giansiracumt/netcdf2ascii/blob/master/netcdf2ascii#L261
super nit pick, but generally the Python world prefers snake_case over camelCase for variable and function definitions (class names being the exception)
Generally in your file handling, use the pathlib stdlib. It'll make globbing and handling path building and manipulation much easier
Add a setup.py file to make this `pip install`ablehttps://code.arm.gov/giansiracusa/netcdf2ascii/-/issues/2Filter DQR file2018-09-04T13:40:58ZMichael GiansiracusaFilter DQR fileFilter the DQR file that is written to the ascii_csv directory so that it only includes the time range selected for that datastream. Check that append will still work for multiple datastreams.Filter the DQR file that is written to the ascii_csv directory so that it only includes the time range selected for that datastream. Check that append will still work for multiple datastreams.Michael GiansiracusaMichael Giansiracusa