Feature: Allow custom CF url (-d) #1066
Conversation
|
Will go back and tackle the lint errors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1066 +/- ##
===========================================
- Coverage 82.18% 81.99% -0.20%
===========================================
Files 24 24
Lines 5171 5181 +10
Branches 1242 1248 +6
===========================================
- Hits 4250 4248 -2
- Misses 622 633 +11
- Partials 299 300 +1 ☔ View full report in Codecov by Sentry. |
* Convert print statements to f-strings
|
We will look into adding some more tests to satisfy codecov. In the meantime, the pre-commit.ci failure is in conflict with a local run of pre-commit. Result from pre-commit.ci: Local run: Will try: pre-commit.ci autofix to see what that does. |
|
pre-commit.ci autofix |
|
@jcermauwedu I would ignore pre-commit-ci for now. It seems to be a bit wacky. I'll try to update/config it better in the near future. Let's focus on your change to the code base here so we can get this reviewed and merged ASAP. |
| @@ -19,7 +19,7 @@ jobs: | |||
| create-args: >- | |||
| python=3 pip | |||
| --file requirements.txt | |||
| --file test_requirements.txt | |||
| --file requirements-test.txt | |||
There was a problem hiding this comment.
Changes like this make it hard to follow the actual changes in the PR. While I personally prefer the requirements-dev.txt we are stuck with the original file name here and no need to change it now.
PS: I'd welcome a change like this in its own PR, away from important code changes that require careful review.
| regex | ||
| requests | ||
| shapely | ||
| validators |
There was a problem hiding this comment.
Why do you need to remove the lower pinned version? Most, if not all of those, if they make into an environment, will likely break compliance-checker. Upper pins are bad IMO but lower are fine, they not only ensure a stable dev env but also increase solver speed for most package managers.
| url = f"http://cfconventions.org/Data/cf-standard-names/{version}/src/cf-standard-name-table.xml" | ||
| if version.startswith('http'): | ||
| url = version | ||
| version = '"url specified"' |
There was a problem hiding this comment.
Maybe we could say "custom URL"? Although, if this variable is not used anywhere we can probably remove it here.
compliance_checker/cf/util.py
Outdated
| @@ -289,7 +289,13 @@ def download_cf_standard_name_table(version, location=None): | |||
| if version == "latest": | |||
| url = "http://cfconventions.org/Data/cf-standard-names/current/src/cf-standard-name-table.xml" | |||
| else: | |||
| url = f"http://cfconventions.org/Data/cf-standard-names/{version}/src/cf-standard-name-table.xml" | |||
| if version.startswith('http'): | |||
There was a problem hiding this comment.
@jcermauwedu it seems that your core changes are in this commit, right? This is an easy merge IMO if you don't mind submitting just that in this PR and the rest in a linting one.
|
Thank you for the feedback. |
|
@jcermauwedu would it make sense to also allow for local xml files? I don't have a use case for that but maybe we should not restrict it here to files on the web. |
This makes sense too. There is a lot of chicken and egg things going on here. The checker is initialized before the arguments are parsed. When the checker is initialized it has already read the standard names file. Then the arguments are processed and updates the file on the disk, but the test will use what has already been read in memory. IE: We have to find a way send a signal to the tests that the user desired a different standard name table. Or defer loading of the standard table in the class right up until the first test is run. Somewhere... I was starting to implement a I have reverted the other items and will put in a separate PR if needed. Pinning packages on the minimum side makes sense. I am also learning a bit about python coverage. This PR might extend into the sprint. |
That is a bug IMO. If we can tackle that during the sprint, it is a success already. (Although we have a lot in our plates for this sprint already ;-p) |
|
Turns out this feature already exists via an environment variable. We will revise the PR to provide additional documentation to highlight the feature. The code could be improved to see if there is an http or https prefix to attempt a remote url fetch. |
|
Do we want to close this as not planned in favor of documentation? |
Allow for a custom CF url for checking standards. Allows for checking files with a custom CF xml file with proposed changes to ensure operability prior to adoption.
Example of use:
This comes with a bug to track down: the program needs to be run twice. Once to download the custom XML and a second time to actually use it. So, there is actually a bug when requesting a different version of the CF standard names as well.