search

xproc / 3.0-specification

A community-driven effort to define an XProc 3.0 specification (formerly 1.1)
http://spec.xproc.org/
33 stars 10 forks source link

Improve the description of p:import #1088

Closed ndw closed 5 months ago

ndw commented 7 months ago

Close #1083

ndw commented 7 months ago

We appear to have lost the feature that published links to the formatted PRs. 😦

You can find this one at https://spec.xproc.org/pr/1088/xproc/ with diffs.

ndw commented 7 months ago

I think this PR is almost entirely editorial. The single non-editorial change is that err:XS0053 is extended to cover the case where a p:declare-step is imported and it specifies visibility="private". That seems consistent with saying that a declared step without a type is an error. If we don't make it an error then either the import is pointless (in which case, why are we making the lack of a type an error) or we have to say that the visibility is ignored in this case. Making it an error seems cleaner to me.

xml-project commented 7 months ago

I would review this request, if I only could. Somehow I can merge or close the PR, but not review it. Strange. I agree that this is almost entirely editorial and clears up import. Concerning the extension of XS0053 I am not sure: My implementation ignored imports of steps declared as private (with printing out a warning). This change would therefor break pipelines currently running. On the other hand I agree that your proposal is cleared.

ndw commented 7 months ago

(I don't know how/why only the "spec authors" team can review PRs, but I decided adding you to that team was the shortest path to fixing the problem. You should have received an invite a few minutes ago.)

ndw commented 7 months ago

I don't feel strongly about changing err:XS0053.

It strikes me as an unlikely edge case for backwards compatibility, though. The pipeline that will break is one that uses p:import to import a single p:declare-step that's marked as private, so importing it has no effect.

If we wanted to avoid introducing the backwards incompatibility and make the behavior consistent, we could go the other way, and remove err:XS0053 saying simply that importing a p:declare-step that has no type has no effect. That would be perfectly fine to me too.

xml-project commented 7 months ago

I don't feel strongly about changing err:XS0053.

Me neither.

It strikes me as an unlikely edge case for backwards compatibility, though. The pipeline that will break is one that uses p:import to import a single p:declare-step that's marked as private, so importing it has no effect.

Yes.

If we wanted to avoid introducing the backwards incompatibility and make the behavior consistent, we could go the other way, and remove err:XS0053 saying simply that importing a p:declare-step that has no type has no effect. That would be perfectly fine to me too.

I could go with that too. A processor could print out a warning in both cases.

Not sure, which I would prefer. @gimsieke @xatapult Any opinion on this?

gimsieke commented 7 months ago

Sorry, I can’t offer opinions today or look into the matter at all. I’ll let you decide.

ndw commented 7 months ago

One more observation that might support removing the error is that it currently isn't an error to import a p:library that contains a single p:declare-step that has no type. (Having steps without types in a library is pointless, I think, but we don't make it an error.) That's arguably "the same" as importing the p:declare-step directly, so it shouldn't be an error in the second case.

I'd be mildly opposed to making it an error for libraries to contain steps that don't have types. It strikes me as a convenient way to "comment out" a step while still making the processor confirm that the step is correctly declared. And it would be backwards incompatible in a potentially annoying way.

xml-project commented 7 months ago

@ndw: Good point. So we get rid of XS0503 completely, right?

ndw commented 7 months ago

I don't feel like we need to resolve the issue urgently, but yes, I think that's the way the winds of consensus are blowing.

xml-project commented 7 months ago

Related: Spec says:

If the p:declare-step is not a child of a p:library the attribute has no effect and is ignored.

If we go for the "error"-variant, we have to change this.

ndw commented 7 months ago

I think consensus was moving in the direction of removing err:XS0053, so I've updated the draft. I also added an erratum section explaining the changes.

It occurs to me that it might be possible to automatically generate an errata appendix from those sections, saving us from having to create a separate errata document.

ndw commented 5 months ago

The point of errata, of course, is that they're linked from the original specification. We don't need to summarize errata in the new draft.

I've pulled that section out and started working on an actual errata document.

I'd like approval to merge the changes now, I guess.