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

Making file type an optional argument #155

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Making file type an optional argument #155

merged 2 commits into from
Sep 21, 2022

Conversation

kellrott
Copy link
Member

@kellrott kellrott commented Sep 2, 2021

No description provided.

@kellrott kellrott added this to the 1.1 milestone Sep 2, 2021
description: |-
Define if input/output element is a file or a directory. It is not required
that the user provide this value, but it is required that the server fill in the
value once the information is avalible at run time.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo with "avalible"

@aniewielska
Copy link
Contributor

If input/output types are not mandatory, should they have a default value then? How does a default behave for an optional parameter?
I will check a code generator for this, as I expect problems.

@aniewielska aniewielska self-requested a review February 14, 2022 12:26
Copy link
Contributor

@aniewielska aniewielska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure of the behaviour of the default values for optional parameters, especially in the context of generated code. I will check it with code generation.

@kellrott kellrott changed the base branch from develop-1.1 to develop April 20, 2022 19:49
@uniqueg
Copy link
Contributor

uniqueg commented Jul 19, 2022

Any feedback on your tests, @aniewielska? Intuitively I would think that handling defaults should be pretty standard for any validator and code generator based on JSON Schema...

@aniewielska
Copy link
Contributor

I think at the moment the type is still mandatory, if we leave defaults with optional. I think the spec was wrong previously in combining defaults and required. But the behaviour we want is no default. When no type of in/output is provided the implementation needs to figure it out itself and not assume it is FILE.

@uniqueg
Copy link
Contributor

uniqueg commented Aug 24, 2022

Given that the accepted values are defined in an enum, the value passed for tesFileType has to be a value matching one of the enumerated values. If there's only FILE and DIRECTORY in the enum, one couldn't send a valid request omitting this if there's no default set. I would propose adding UNSET or UNSPECIFIED or something to that effect to the enum and then use that as default.

EDIT: I kinda lost track of the point here - that tesFileType is now optional. Not sure how that works with a default value, but I would assume that the expectation is that the default is still set in every request if no other value is supplied - which is not what we want. Removing the default may fix that. But perhaps adding UNSPECIFIED or similar to the enum and then setting that as default may make the intention clearer.

@aniewielska
Copy link
Contributor

Right. So, if we cannot have null enums, I vote for adding UNSPECIFIED and making it default as it nicely described th intention here that the caller would not like to specify the type.

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.

6 participants