kbase / sample_service_validator_config

Stores the sample service metadata validation configuration file.
MIT License
0 stars 9 forks source link

changing fields to lower, replacing spaces with "_", adding "display-name" key_metadata #15

Closed slebras closed 4 years ago

slebras commented 4 years ago

The "callable-builder" key is necessary for the config to be readable by the SampleService. The implementation there would have to change.

I think the reason "key_metadata" was chosen because each of these fields represents metadata themselves and the "key_metadata" is specifically metadata about the key name. Not a great name really and "metadata" would probably just be clearer. I think that change would also require alterations to the SampleService to update

good call on the parenthesis. My only concern about removing slashes is that they have a little more meaning, but I'm sure that they will probably end up being more of a headache down the line than they are worth

MrCreosote commented 4 years ago

In the validation config, what is "callable-builder" as differentiated from a function or method?

It's a function that builds a callable. See https://github.com/kbase/sample_service#configuration

Also "key_metdata" is a bit redundant, how about just "metadata?", since everything under a key is about the key?

I don't think it is redundant - it is metadata about the key, as opposed to metadata under the key / metadata to which the key maps. That being said, I'm not fond of the name either and only used it because I couldn't think of anything better. Suggestions welcome, but just metadata is too ambiguous.

Also, how about using one form of key in all the configs, when possible. Thus, e.g., using snake_case rather always than sometimes using kebab-case.

Crap. Didn't notice I'd done that. https://github.com/kbase/sample_service/issues/285

MrCreosote commented 4 years ago

@slebras want to review? https://github.com/kbase/sample_service/pull/286

I'll restart the service, then you can update the config file, and then I'll take out the alternate form

MrCreosote commented 4 years ago

BTW you know you can use YAML syntax to reference other parts of the yml document and not repeat the same structure over and over again right?

eapearson commented 4 years ago

key_metadata - I don't really care, it is just a name in the end, so feel free to ignore that comment!

It seemed to me that calling it "metadata", for instance, is perfectly clear, since a property should be interpreted in its scope, and that scope is a field's definition. the key_ in key_metadata confused me, because I'm thinking there must be multiple kinds of metadata for a field definition, but no, it is all the metadata. And a key is just an identifier for a field, so metadata about a key doesn't make sense to me either.

MrCreosote commented 4 years ago

It seemed to me that calling it "metadata", for instance, is perfectly clear, since a property should be interpreted in its scope,

Since the key is a metadata key, it'd be easy for someone to think that a sibling metadata key was the actual metadata stored under the key. Lots of room for misinterpretation. Even key_metadata is too easy to misinterpret IMO.

And a key is just an identifier for a field, so metadata about a key doesn't make sense to me either.

It does to me - it's data about the semantics of the key. The key is data, so it's metadata.

Again, I don't like the name, and if anyone has better suggestions I'd be happy to consider them.

slebras commented 4 years ago

changed callable-builder -> callable_builder and display-name -> display_name

slebras commented 4 years ago

@MrCreosote @eapearson I think its ready to merge, any objections?

MrCreosote commented 4 years ago

I don't have objections, but I haven't reviewed it either. I trust you and Erik to get it right.