usgs / groundmotion-processing

Parsing and processing ground motion data
Other
54 stars 41 forks source link

Clean up use of pathlib.Path #1020

Open baagaard-usgs opened 1 year ago

baagaard-usgs commented 1 year ago

There are lots of places where we have Path(filepath) where filepath is a pathlib.Path. This is unnecessary. Once we have a pathlib.Path object, additional operations produce pathlib.Path objects.

All of the following produce pathlib.Path objects.

filepath1 = filepath.parent
filepath2 = filepath / "subdir"

Additionally, we should replace use of os (e.g., os.join, os.makedirs, etc) with pathlib.Path methods. The code is more concise and easier to read.

emthompson-usgs commented 1 year ago

I think some of the difficulty is that we never fully migrated to pathlib from os. So in some places we need the string representation of the path, and that is how it is stored in the gmrecords object. We should change it so that the path is always a pathlib object, including where it is as an attribute of gmrecords.

Thanks for self assigning @smithj382.

ghost commented 1 year ago

It looks like @baagaard-usgs took care of this issue when he refactored the projects subcommand? Thanks if that is indeed the case.

baagaard-usgs commented 1 year ago

There are still lots of places in the code that use os for filesystem path operations.

emthompson-usgs commented 1 year ago

I think I'm going to have to deal with this as part of #1005/#1028. In cleaning up the test data, I realized there was some duplicated code in different places for flattening multiple levels of compression. Also, we made heavy use of os.path for that code so I changed it to pathlib. But then a bunch of tests that were passing started failing, but only on windows. I suspect that it is related to our inconsistent use of pathlib and os.path, so I think I'm going to have to resolve this before I merge those changes.

emthompson-usgs commented 1 year ago

I don't think this is a bug, so I removed that label. Also, I think this was largely resolved in #1028.