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

Repeated parameters improvements #680

Merged
merged 3 commits into from
Jan 6, 2019

Conversation

NicolasMahe
Copy link
Member

@NicolasMahe NicolasMahe commented Jan 6, 2019

Improvement of #679

  • Use type casting instead of reflection
  • Update array test types from []string to []interface{}
  • Update test to use data from JSON rather than go struct. That way, we don't choose any type, but let json.Unmarshal decide.

This was referenced Jan 6, 2019
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

looks ok to me

Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

We should also support other slice types like []string, []bool, []int and so on. Otherwise Validate() will depend on to get data that preduced by a json.Unmarshal(). Because only unmarshalling produces slices as []interface{}. Either we should document this or support other slices as well.

But if we don't support all slices we'll face the same problem hapenned here. We'll need to manually change types to []interface from other slice types when we work with system services because json.Unmarshal() isn't used there.

@antho1404
Copy link
Member

For now we need only to support []interface{}, let's just support that and see if one day we need to support something else we can improve that. Here we just focus on validation of these data that are always from json.
We can add a comment to make sure that there is a different way to do it but for now we don't need it for this feature

@NicolasMahe NicolasMahe merged commit d67df23 into feature/repeated-params Jan 6, 2019
@NicolasMahe NicolasMahe deleted the feature/repeated-params-nico branch January 6, 2019 13:38
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.

4 participants