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

Add default values for options that are maps #165

Closed
ndw opened this issue Jul 16, 2019 · 10 comments

Comments

@ndw
Copy link
Collaborator

commented Jul 16, 2019

We have several steps with map-type options, e.g. serialization.

Currently, they don't specify a default value so they get the default value (). That doesn't work. I think the easiest solution is to give them all default values of map { }.

@xml-project

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

👍
But isn't that a step issue as only option default to ()? Did I miss the passage where we say, that attributes like p:output/@p:serialization also default to ()?

@ndw

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

It is a step issue, the particular step I was implementing was p:escape-markup :-)

I don't think we have to do or say anything about maps in places like p:output because they're part of the processor.

@xml-project

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Sorry, I though you raised that error in the core specs repo.
Also: I was surprised to learn that () instance of map(*) is false. But it is!

@gimsieke

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Maybe we can specify map options as "map(xs:Qname, item()*)? instead of "map(xs:Qname, item()*), then () is a valid default

@ndw

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

That would make empty sequence valid, but it would also mean that every step implementation has to check if the value it received is an empty sequence. I think it's simpler if the default is an empty map.

@xml-project

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

It seems to me, that the solution proposed by @gimsieke is widely used in the step library currently. But you really think we need to change that?

@ndw

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2019

🤷‍♂ Fine by me, I suppose

@xml-project

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@ndw Did I miss something?

@ndw

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2019

It's fine. It's just slightly more work in the implementation of each step because you have to check for empty sequence instead of having an empty map provided automatically. But it's probably more consistent and simpler for users, so I'm fine with it. I wrote a convenience method 😄

@ndw

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 24, 2019

I think this is closed by #171 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.