Update split list #89
Conversation
There was a problem hiding this comment.
Thanks @j-s-135! I appreciate you foreseeing the potential merge conflicts with my PR and like how you've proposed reconciling them. Overall I think things look good (nice shout out to @trumbullm in your PR description, by the way 😉 ), but just had a few suggestions.
Also, since this is a pretty small set of changes, can you change the PR base to my branch (dev-dwp-ihm)? (You should just be able to click "Edit" at the top of your PR page and select my branch instead of master.) That way, we can just incorporate this along with my changes in a single version bump with my PR.
| parser.add_argument("--outputContentType", action="store_true", default=False, help="Whether output path in downstream application has prepended content type (pdb, csm) before file name") | ||
| parser.add_argument("--outputHash", action="store_true", default=False, help="Whether output path in downstream application has prepended hash before file name") |
There was a problem hiding this comment.
Can you change the argument to be snakecase instead of camel, just to be consistent with the other CLI argument names? (I know, it's annoying that these CLI ones are snake while everything else is camel, but it's too late to change everything now...)
There was a problem hiding this comment.
Also, since these are flags, I might suggest prepending both arguments/variables with prepend, to make it more clear that they are flags and not input parameters, e.g.:
--prepend_output_content_type (corresponding var: prependOutputContentType)
--prepend_output_hash (corresponding var: prependOutputHash)
| return "csm" | ||
| return "" | ||
|
|
||
| def getTimeStampCheck(self, hD, targetFileDir, targetFileSuffix, databaseName, outputContentType=False, outputHash=False): |
There was a problem hiding this comment.
I know you didn't originally write this, but could you add a brief docstring describing what this does? From my understanding, it could say something like this (please correct me if I'm wrong)?:
Filters a list of structure identifiers to return for post-processing by comparing the
timestamp of the source file (based on holdings file information) with the timestamp
of the target file (based on the actual file properties). If the target file exists and
is newer than the source timestamp, then that identifier is removed from the returned
list for processing.
| # experimental models are stored with lower case while csms are stored with upper case (except content type) | ||
| pdbid = key.lower() | ||
| contentTypePrefix = self.getContentTypePrefix(databaseName) | ||
| hashPath = self.getPdbHash(pdbid) |
There was a problem hiding this comment.
Since this is specific for experimental PDBs, I'd add a conditional above here checking that, e.g. if databaseName == "pdbx_core", just like you do below for CSMs.
| @@ -26,6 +26,7 @@ | |||
| # Add support for logging output to a specific file | |||
| # 25-Apr-2024 - dwp Add support for remote config file loading; use underscores instead of hyphens for arg choices | |||
| # 22-Jan-2025 - mjt Add Imgs format option flags | |||
| # 5-Mar-2025 - james smith output hash | |||
There was a problem hiding this comment.
sorry, my internal linting is kicking in and I can't control it
| # 5-Mar-2025 - james smith output hash | |
| # 5-Mar-2025 - js Add support for prepending content type and directory hash for splitIdList output |
Add hash and capitalization to the already great timestamp function.