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

Fix error code #344

Closed
wants to merge 1 commit into from
Closed

Fix error code #344

wants to merge 1 commit into from

Conversation

@ndw
Copy link
Contributor

ndw commented Dec 17, 2019

Fix #343

I don't expect this to be controversial.

@ndw ndw requested a review from xml-project Dec 17, 2019
@xml-project xml-project self-requested a review Dec 18, 2019
Copy link
Contributor

xml-project left a comment

Are you sure about this? I think this is the use case for XC0081: A format is specified and the document is not an archive of this type:

It is a dynamic error (err:XC0081) if the format of the archive does not match the format as specified in format option.

@xml-project xml-project self-requested a review Dec 18, 2019
Copy link
Contributor

xml-project left a comment

Sure? I this is exactly the use case for this error: Format is specified but the document is not of this type:

It is a dynamic error (err:XC0081) if the format of the archive does not match the format as specified in format option.

@ndw

This comment has been minimized.

Copy link
Contributor Author

ndw commented Dec 18, 2019

I'm confused.

The heart of ab-p-archive-056 is this step:

            <p:archive>
               <p:with-input port="source"><p:empty /></p:with-input>
               <p:with-input port="manifest"><p:empty /></p:with-input>
               <p:with-input port="archive">
                  <p:inline content-type="text/plain">This is no zip.</p:inline>
               </p:with-input>
            </p:archive>

the heart of ab-p-archve-057 is this step:

            <p:archive format="zip">
               <p:with-input port="source"><p:empty /></p:with-input>
               <p:with-input port="manifest"><p:empty /></p:with-input>
               <p:with-input port="archive">
                  <p:inline content-type="text/plain">This is no zip.</p:inline>
               </p:with-input>
            </p:archive>

The default value for the format attribute on p:archive is "zip". So I don't see how these two tests can possibly be expected to give different results.

@xml-project

This comment has been minimized.

Copy link
Contributor

xml-project commented Dec 18, 2019

Right, but 056 does not have a format option and this makes the difference:

format present and document is not format -> XC0081
no format present and document not processable -> XC0085

I have not invented this distinction but I think the idea is this: Suppose I support "zip" and another archive type "x". If format = 'zip', but document is of type 'a', XC0081 has to be raised. XC0085 is not allowed, because I support 'a'.
What did I miss?

@ndw

This comment has been minimized.

Copy link
Contributor Author

ndw commented Dec 18, 2019

I'm still confused. The default value of the format attribute is "zip". And the format attribute is not optional. AFAICT, it is not possible for the step to run with a format attribute that is "not present".

@xml-project

This comment has been minimized.

Copy link
Contributor

xml-project commented Dec 18, 2019

And the format attribute is not optional.

To my reading it is optional:

<p:option name="format" as="xs:QName" select="'zip'"/>

What did I miss?

@ndw

This comment has been minimized.

Copy link
Contributor Author

ndw commented Dec 18, 2019

It's optional for the pipeline author. But if the author doesn't provide a value, the default value is "zip". So from the perspective of the step implementation, the format attribute always has a value. It can't not have a value.

What am I missing?

@xml-project

This comment has been minimized.

Copy link
Contributor

xml-project commented Dec 18, 2019

Now I see your point. You are right.
But does the distinction between the two errors make any sense? I took it to mean, that we have to make a distinction where 'zip' is given and 'zip' is only assumed, but not state it.

We should talk about this at tomorrows call, so the inventer of the distinction can enlight us.

@ndw

This comment has been minimized.

Copy link
Contributor Author

ndw commented Dec 18, 2019

I'd be happy to combine them into a single code. I struggled to work out which one to raise and there are a few places where I have extra tests to raise one when the other would have been raised a moment later.

@xml-project

This comment has been minimized.

Copy link
Contributor

xml-project commented Dec 18, 2019

Same here!

@ndw

This comment has been minimized.

Copy link
Contributor Author

ndw commented Dec 20, 2019

Subsumed by #352

@ndw ndw closed this Dec 20, 2019
@ndw ndw deleted the ndw:iss343 branch Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.