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

service/importer: support numbers in names, closes #396 #493

Merged
merged 3 commits into from
Sep 24, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Sep 24, 2018

No description provided.

@ilgooz ilgooz requested a review from a team September 24, 2018 05:41
@antho1404
Copy link
Member

I really don't like to have the possibility to start a key with a number.

We could have some parameter/output/task... with something like 01-taskX this might create some problems with some programming language because of variable name that starts with number. Also what happen if we have only number as a key, like 0, 1, 2, 3 ? Some parsers might parse this as an array and not a map.

I'm ok to accept numbers in keys but it should be forbidden to start with a number.

^[a-zA-Z]+[0-9a-zA-Z_-]*$ something like that we need to think about it

@krhubert
Copy link
Contributor

krhubert commented Sep 24, 2018

We could have some parameter/output/task... with something like 01-taskX this might create some problems with some programming language because of variable name that starts with number.

If we want to consider programming languages rules then we should take a common set of variable names rules in a major programming language.

^[a-zA-Z]+[0-9a-zA-Z_-]*$ something like that we need to think about it

- is invalid in variable name in most of programming languages and we are using it right now.

I think we should have really good reason to forbid some task names otherwise I think regex ^[0-9a-zA-Z_-]+$ is totally fine.

.... Some parsers might parse this as an array and not a map.

No, they can't. Both JSON and Yaml have different syntax for array and map (no matter what keys names are inside)

@ilgooz
Copy link
Contributor Author

ilgooz commented Sep 24, 2018

I agree with @krhubert, but also ok with both keeping or restricting numbers from the beginning of names.

mesg.yml should only be parsed by core/service/importer/. If someone wants know what is defined in inside the yml then they can make a GetService() request via API. This way, having numbers in the beginning will not make any problem, because we already choose the data type with protobuf definitions.

@antho1404
Copy link
Member

Ok agree let's be flexible for now we can always add more check later on if it's really needed

@ilgooz ilgooz merged commit 50bf400 into dev Sep 24, 2018
@ilgooz ilgooz deleted the feature/yml-numbers branch September 24, 2018 12:12
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.

3 participants