search

fvutils / pyucis

Python API to Unified Coverage Interoperability Standard (UCIS) Data
https://fvutils.github.io/pyucis
Apache License 2.0
21 stars 12 forks source link

quick hack to have validate_ucis_xml return true/false #6

Closed nsdjg closed 2 years ago

nsdjg commented 2 years ago

This is a work in progress WIP. I'm doing this in conjunction with this [fc4sc pull request(https://github.com/amiq-consulting/fc4sc/pull/23)

mballance commented 2 years ago

I'd be happy to accept this if the patch is just the addition of line 65. I'm unclear as to why all the whitespace diffs show up in the PR.

nsdjg commented 2 years ago

I just adjusted to have a single line diff. Github should auto pickup the mod.

The white space was because in my editor its configured to highlight any whitepace. Its kind of annoying. I didn't set it up that way. I believe it will flag lint warnings. I can assist you with lint warnings later if you wish.

Lastly, I am impressed with your makefile auto setting up venv so that it can build. Iv'e never seen that before and I hope to use your setup as a reference in fc4sc similarly. Good job on that!

nsdjg commented 2 years ago

btw, here is where github autopicked up the merge per your request for mod.

image

nsdjg commented 2 years ago

Thanks, @nsdjg. This looks good

You are welcome @mballance

nsdjg commented 2 years ago

Matthew, I was talking to Ed offline about this pull request. There is some consternation regarding the False|None+exception vs True|False return. Being that I have been criticized in the past regarding my python looking like C, or my matlab looking like C, Swift looking like C, or just about any programming not looking pythonic, matlabish, Swifty, etc, I took this as an opportunity to read the docs to see what is the preferred approach.

With that said, I did not find anything definitive. The closest I found is about the 15th bullet in this section of the PEP. There is an example of wrong vs correct on how to return a float result. When the calculated result is invalid they don't raise an exception, they return None.

So, I don't know which is best and I don't want to be pedantic. I noticed you did not catch the exception so perhaps you are following that method. I apologize if it seems that I am being over pedantic.

Lastly you have not accepted/closed the pull request so perhaps you are mulling it over. I strive for flexibility so please let me know which style you prefer and I will adjust as you desire. Like I said, I am just trying to resolve it so that I don't pip install from a git repo. If you deem it unnecessary change, I'll modify the unit test case in FC4SC to assert on a raised exception instead of True/False. Anyway, just dropping you a line to say I looked further into the matter.

mballance commented 2 years ago

Hi John, apologies about the pull request. I had to hand-merge your final commit for some reason in order to avoid the whitespace issue. If you have a look at the 'Files changed' tab on the PR, you'll see that it still shows the whitespace. You can find the commit with your updates here: https://github.com/fvutils/pyucis/commit/fd850ff1cbba43154371274e35569faba426c06f For future PRs, I'd suggest creating a new branch in your repo with just the PR changes instead of creating a PR from 'main'.

You raise a good point about a method that both returns a value and that may raise an exception. Probably best to only do one. In this case, because the exception contains information on the source of the validation failure, I'd prefer to raise the exception. Will that work for you?

nsdjg commented 2 years ago

Hi John, apologies about the pull request. I had to hand-merge your final commit for some reason in order to avoid the whitespace issue. If you have a look at the 'Files changed' tab on the PR, you'll see that it still shows the whitespace. You can find the commit with your updates here: fd850ff For future PRs, I'd suggest creating a new branch in your repo with just the PR changes instead of creating a PR from 'main'.

You raise a good point about a method that both returns a value and that may raise an exception. Probably best to only do one. In this case, because the exception contains information on the source of the validation failure, I'd prefer to raise the exception. Will that work for you?

Yes, I can use a separate branch. Yes, I can test for an exception or False. Thank you for the quick reply. You are quick. Many thanks.