-
Notifications
You must be signed in to change notification settings - Fork 88
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
Unihan phase2 #1901
Unihan phase2 #1901
Conversation
64a3b34
to
64a895a
Compare
So it is telling me that my build is invalid because the elements i m trying to deprecate are going to be deprecated. If there is another error i don't see it. I need help here. |
@duncdrum Yeah—looks like your Schematron tests are flagging every example in the Guidelines featuring the code you are deprecating. That is probably good, but do we need to do some editing of those examples to indicate they are invalid? I guess we would not want to remove them entirely because they can now help guide people to the new code. |
And they are not supposed to be invalid until 2021. Do I maybe need to edit specific occurrences? |
@duncdrum No time to check, but do you have a good validUntil date on your deprecations? They should be ignored by the build until the validUntil has expired. |
|
this concludes phase 2 from my side. It incorporates some Phase 5 changes since deprecating the children without changing the parent didn't work. Now on to the real fun. |
While looking at this, the checks have passed now. What was the reason for the error? |
death by a thousand paper cuts. Normalizing the deprecations and adding |
deployed at https://jenkins.tei-c.org/job/TEIP5-branch-unihan/lastSuccessfulBuild/artifact/P5/release/doc/tei-p5-doc/en/html/index.html for facilitating review |
Thank you @peterstadler. Will this automatically update? I already found some prose typos, and my use of |
Just briefly regarding the Jenkins setup: Yes, it will build automatically from your duncdrum:unihan-phase2 branch. If you create phase3, please drop me a line ;) |
could someone please comment on the way that |
Is the classSpec getting included correctly in the output from odd2odd? |
@lb42 good questions, what is the output of odd2odd? Is it run as part of |
going to rebase and push some minor fixes since my build PR has been merged in he meantime. |
Odd2odd generates a set of complete declarations corresponding to your odd. It should be run by make test but I think the output is deleted after use. |
The only difference I noticed when comparing with other attribute class definitions is the |
It's the default value. See the spec for attList. |
There is definitely something fishy about the processing of change. The class defines three attributes ( Apart from testing each attribute solo, i have tried:
This seems to be something @TEITechnicalCouncil might want to take a look at, but its not really a uniHan related problem. |
If there is anything that i can do to help with review let me know. I think the new elements and removal of faulty info from the GL would actually make a good addition to upcoming release. Phase 3-6 are not really necessary for that. |
soo ? |
Revert the datatype of the name= attr of <unicodeProp> to xmlName (from enumerated) per discussion on PR TEIC#1901. Council may decide they should both be xmlName or they should both be enumerated, but clearly if the enumerated values meet the same restrictions, the defintions of an attribute on <unicodeProp> and on <unihanProp> should match.
38dc5b5
to
b18994b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not done reviewing, but done for now. Hope to do more of WD tomorrow.
Co-Authored-By: Syd Bauman <sydb@users.noreply.github.com>
Co-Authored-By: Syd Bauman <sydb@users.noreply.github.com>
Co-Authored-By: Syd Bauman <sydb@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I’m done. (At least for now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I’m done. (At least for now.)
ok i m going to make some final tweaks and do another rebase that squashes the review commits. |
c9baa78
to
a5f5591
Compare
some comments outstanding, will fix this manually Co-Authored-By: Syd Bauman <sydb@users.noreply.github.com>
a5f5591
to
f0d3748
Compare
So … I went to try to see what was done with my various change requests, but I can’t figure out how. Is that because GitHub just doesn’t have an interface for that, or because @duncdrum did another rebase that squashed the review commits 4 days ago? |
You can check the batch of changes by looking at the last commit, or you have to look through Each outdated conversation marker in the history up here |
I’ve glanced at all of @raffazizzi’s proposed changes, and the English ones all look good to me. |
Thank you for the review. All in. Co-Authored-By: Raffaele Viglianti <raffaeleviglianti@gmail.com>
@sydb thanks for looking this over again! Could you approve your review today? If I don't hear back by mid-day EST 2020-02-07, I'll go ahead and merge. |
Revert the datatype of the name= attr of <unicodeProp> to xmlName (from enumerated) per discussion on PR #1901. Council may decide they should both be xmlName or they should both be enumerated, but clearly if the enumerated values meet the same restrictions, the defintions of an attribute on <unicodeProp> and on <unihanProp> should match.
this obviously extends #1900
i can't get the deprecation to work though, from past commits i understand that I should update the expected results, but no matter how i copy, move, teleport the unopened
validatorLog.xml
it doesn't like it.If anybody sees where my error lies I'd welcome the feedback. Should I maybe remove the examples on the spec pages which have the deprecation warning?
Phase 2 off #1805
close #1804