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

Remove tag ordering restriction #29

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

h3xx
Copy link

@h3xx h3xx commented Jul 12, 2022

Let the document specify the tags immediately under <ComicInfo> in any order, instead of in sequence.

The XSD should allow BOTH forms of:

  • <Series>...</Series><Title>...</Title>
  • <Title>...</Title><Series>...</Series>

Should validate:

<ComicInfo>
    <Series>Series 1</Series>
    <Number>Number 1</Number>
    <Title>Title 1</Title>
</ComicInfo>

Should not validate:

<ComicInfo>
    <Title>Title 1</Title>
    <Title>Title 1</Title>
</ComicInfo>

Closes #10

@ThePromidius
Copy link
Contributor

Shouldn't maxOccurs="unbounded" be "1"?

@h3xx
Copy link
Author

h3xx commented Jul 12, 2022

Shouldn't maxOccurs="unbounded" be "1"?

No, maxOccurs="1" limits it to one element, i.e. EITHER <Title>...</Title> or <Series>...</Series>

I tried validating

This document
<ComicInfo
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="https://github.com/anansi-project/comicinfo/raw/main/drafts/v2.1/ComicInfo.xsd"
>

    <Title>Title 1</Title>
    <Series>Series 1</Series>
    <Number>Number 1</Number>
    <!-- XXX Count of pages? Count of issues in series? -->
    <Count>1</Count>
    <Volume>1</Volume>
    <AlternateSeries>AlternateSeries 1</AlternateSeries>
    <AlternateNumber>AlternateNumber 1</AlternateNumber>
    <AlternateCount>1</AlternateCount>
    <Summary>Summary 1</Summary>
    <Notes>Notes 1</Notes>
    <Year>2000</Year>
    <Month>1</Month>
    <Day>1</Day>
    <Writer>Writer 1</Writer>
    <Penciller>Penciller 1</Penciller>

    <LanguageISO>LanguageISO 1</LanguageISO>
    <AgeRating>Everyone</AgeRating>
    <Format>Format 1</Format>
    <!-- Acceptable (case-insensitive): yes, true, 1 -->
    <BlackAndWhite>Yes</BlackAndWhite>
    <Genre>Genre 1</Genre>
    <Imprint>Imprint 1</Imprint>
    <Locations>Locations 1</Locations>
    <Manga>Manga 1</Manga>
    <PageCount>PageCount 1</PageCount>
    <Publisher>Publisher 1</Publisher>
    <ScanInformation>ScanInformation 1</ScanInformation>
    <SeriesGroup>SeriesGroup 1</SeriesGroup>
    <StoryArc>StoryArc 1</StoryArc>
    <Teams>Teams 1</Teams>
    <Web>Web 1</Web>
    <Characters>Characters 1</Characters>

    <!-- Comma-separated text, more than one of each of these can be specified -->
    <Editor>Editor 1, Editor 2</Editor>
    <Editor>Editor 3</Editor>
    <Translator>Translator 3</Translator>
    <Colorist>Colorist 1</Colorist>
    <Inker>Inker 1</Inker>
    <Letterer>Letterer 1</Letterer>
    <Letterer>Letterer 2, Letterer 3</Letterer>
    <CoverArtist>CoverArtist 1</CoverArtist>

    <Pages>
        <Page Type="FrontCover" DoublePage="false" ImageSize="10248" Key="foo" Bookmark="bar" ImageHeight="100" ImageWidth="100" Image="1"/>
    </Pages>
</ComicInfo>

with maxOccurs="1" and it complains about <Series> being there even once:

comicinfo.xml:7: element Series: Schemas validity error : Element 'Series': This element is not expected.

But perhaps you're right about <choice> not being optimal. Let me test if repeated elements work as-expected. Maybe I need to get rid of the <choice>. Let me test.

@ThePromidius
Copy link
Contributor

ThePromidius commented Jul 12, 2022

I just tested with the your sample xml and it had some errors.
I think i confused the tag. I tested in a random online validator and it did indeed present the error you mention.
This was also discussed in #10
I believe choice is not optimal either.
The <choice> indicator specifies that either one child element or another can occur:
Per w3s this implies that only one would be chosen. At least that's how i understand it.

@h3xx
Copy link
Author

h3xx commented Jul 12, 2022

I just tested with the your sample xml and it had some errors.

Yeah, the document I put in the comment still has some validity issues, but using the XSD in this project, I'm working through them. See my updated PR description for some samples.

So, through some more testing, I find I had some issues with the schema I first pushed.

  • The XSD using <sequence> in current main doesn't allow BOTH forms of <Series>...</Series><Title>...</Title> and <Title>...</Title><Series>...</Series>
  • Using <option> in my first push allowed multiple <Title> elements - this is not correct.

I managed to fix both the ordering restriction and the dupe rejection issues in 4c076b1, using <all>

@h3xx h3xx marked this pull request as draft July 12, 2022 17:39
@h3xx
Copy link
Author

h3xx commented Jul 12, 2022

TODO: I want to:
- Help get CI working in order to test this change Edit: Done! (See #30)
- Add a test Edit: Done! (See #30)
- Change the rest of the XSD's to reflect this change Edit: Done!

@ThePromidius
Copy link
Contributor

ThePromidius commented Jul 12, 2022

You can push to main in your fork to run CI that's what i usually do.
It does validate btw.

@h3xx
Copy link
Author

h3xx commented Jul 12, 2022

It does validate btw.

The XSD validates, that's great, but I meant something more in-depth, like "here is a ComicInfo.xml file that should validate using the drafts/v2.1/ComicInfo.xsd schema" and "here is one that should not validate".

@ThePromidius
Copy link
Contributor

Oh that's right i just validate locally so never payed attention to that.

Let the document specify the tags immediately under <ComicInfo> in any
order.
@h3xx
Copy link
Author

h3xx commented Jul 12, 2022

Okay, I brought the change into the main schemas, and wrote a script to run a gauntlet of tests (see #30).

@h3xx h3xx marked this pull request as ready for review July 12, 2022 20:53
@gotson
Copy link
Member

gotson commented Jul 13, 2022

Thanks @h3xx , but as mentioned in #10 no parser really cares about the sequence until now. We also did not change it yet, because it could have adverse impacts, like here.

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.

Replace xs:sequence by xs:all
3 participants