Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional check for QualificationValue #100

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Conversation

apetushok
Copy link
Contributor

@apetushok apetushok commented Apr 23, 2024

for #98

@apetushok
Copy link
Contributor Author

I have verified that both characters are valid and are used to display the date. So this simple check should be enough.

image

@mzeinstra
Copy link
Collaborator

They are valid components but they are not valid when used together.

See https://www.loc.gov/standards/datetime/

the uncertain and approximate qualifiers, '?' and '~', when applied together, are combined into a single qualifier character '%';

So it seems that you can only have one character to denote the certainty.

@JeroenDeDauw
Copy link
Member

JeroenDeDauw commented Apr 23, 2024

@mzeinstra so an input such as 1950?~ should be treated as invalid? Or as if it was 1950% ?

Either way, this PR should add a test that verifies the new behavior for 1950?~ @apetushok

@mzeinstra
Copy link
Collaborator

Depending how strict we want to be 1950?~ or 1950~? and essentially not valid values, we can automattically transform them to 1950%, but is that expected behaviour by the users of this tool?

Either it should transform the string or it should throw an error in my opinion. I am leaning a bit to throwing an error as that is more clean than chaning user inputted values.

@apetushok
Copy link
Contributor Author

Thanks @mzeinstra.
Since there is such support according to the specification #100 (comment)
I think it makes sense for us to add it too.
image
cc @JeroenDeDauw

@mzeinstra
Copy link
Collaborator

Thanks @mzeinstra. Since there is such support according to the specification #100 (comment) I think it makes sense for us to add it too. image cc @JeroenDeDauw

Not entirely true, there are not examples on that page that combine ? and , either ? or ?. So while % means that both the qualifiers ? and ~ apply does not mean that ? in a string is valid.

@apetushok
Copy link
Contributor Author

I think you're right. Seems like the best solution here is to throw an error

@apetushok
Copy link
Contributor Author

Fixed
image

@JeroenDeDauw JeroenDeDauw merged commit 41d9be0 into master Apr 25, 2024
12 checks passed
@JeroenDeDauw JeroenDeDauw deleted the close_flag_fix branch April 25, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants