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

issue deprecation warning when assertion is made without context #2513

Merged
merged 7 commits into from
Mar 16, 2024

Conversation

sydb
Copy link
Member

@sydb sydb commented Dec 15, 2023

Work on #2510.

Since this does not use the @validUntil deprecation mechanism, does it need to be added to Appendix G by hand?

I chose 2024-06-30 as end-of-deprecation date on my own accord. But since there is basically no difference between deprecation and error, here (although I suppose we could add role=warning), should we just go straight to error?

Anyway, it passes all tests on my local system both in & out of docker.

Reviewers: You will definitely want to ignore whitespace if you try comparing code. (Other than the code of <constraintSpec>, it is mostly a kinda pointless activity, IMHO.)

sydb added 2 commits December 15, 2023 08:33
… Schematron, for no apparent reason other than it is mildly easier to read when consistent.
@sydb
Copy link
Member Author

sydb commented Dec 15, 2023

P.S. Since after this almost all Schematron was expressed with a namespace prefix (e.g., <sch:assert>) rather than an explicit namespace binding (e.g. <assert xmlns="http://purl.oclc.org/dsdl/schematron">), I changed the last few, as well. So now all Schematron in the Source/ directory uses namespace prefixes.

raffazizzi and others added 4 commits March 15, 2024 21:57
 * Made deprecation message a warning, not an error.
 * Used 2025-03-15 as the end of deprecation period.
 * Added validUntil= (and a description) to the constraint specification that does the work, so that this shows up in Appendix G.
 * Added test for this in Test/detest.odd.
 * NOTE: The test in detest.odd will not actually fire, apparently because it is a warning not an error. So it will start to fire when we change it to an error on 2025-03-15.
We (the NA subgoup) seemed to think our test was passing without updating expected_results because it was a warning, not an error. But CI server disagrees. Hmmm …
@sydb sydb removed the request for review from hcayless March 16, 2024 10:32
@martinascholger
Copy link
Member

@martindholmes the tests are passing now, could you please have a look at it?

Copy link
Contributor

@martindholmes martindholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. But I really wish we could keep PRs to a single set of changes; this seems to be changing namespace mechanisms and whitespace as well as handling the @context problem. When there's so much change in one batch, it's really hard to review the PR. Could that be a recommendation somewhere?

@martinascholger martinascholger merged commit 9b98511 into dev Mar 16, 2024
3 checks passed
@martinascholger
Copy link
Member

Thanks @martindholmes for the quick review. You are right, different changes should not be tackled all at once. We are trying to stick to our policy better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants