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

[UI] - CLI TYPE - Handle pluralization (aliases) #58

Closed
omesser opened this issue Mar 12, 2022 · 4 comments
Closed

[UI] - CLI TYPE - Handle pluralization (aliases) #58

omesser opened this issue Mar 12, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@omesser
Copy link
Contributor

omesser commented Mar 12, 2022

gto audit:

gto audit registrations should give the same output as gto audit registration
(it doesn't, it's just empty, see #57 )

gto add:

Types are free-text here (no assumptions?) so that's debatable, but I think it's worth singularizing them before adding to artifacts.yaml otherwise we get this unwanted behavior due to common (somewhat natural) cli usage (There's the order issue as well, see #53), where's it's apparently clear both artifacts should be of type model:

> gto add oded model models/mymodel.txt
> gto add oded2 models models/mymodel2.txt
> cat artifacts.yaml
...
oded:
  name: oded
  path: models/mymodel.txt
  type: model
oded2:
  name: oded2
  path: models/mymodel2.txt
  type: models
@omesser omesser added the bug Something isn't working label Mar 12, 2022
@aguschin
Copy link
Contributor

Thanks!

  1. I've implemented gto audit registrations as you suggested, everything works now. I've also supported shortcuts and aliases, like:
class ALIASES:
    REGISTER = ["reg", "registration", "registrations", "register"]
    PROMOTE = ["prom", "promote", "promotion", "promotions"]

Do you think it's fine to support them? I mean, people will rely on them and if we decide to remove some of them in future, it can break user's workflow. On other hand in k8s you also have this kind of shortcuts and they are very helpful.

  1. In fact, model and models may be different things. Maybe I have a model (models/my-model) and models (models/). So not sure we need to assume it's a 100% typo. (This example also touches interleaved paths, but we can discuss it here Can refer to the same filepath by multiple artifacts #59).

I have few options in mind here:
A. Make a spell-check and warn you when you do that, so you can spot in and fix it before you commit this.
B. Allow to specify a whitelist of allowed values. Then if you make a typo, gto will throw an error.
C. Same as A, but if we think this may be a typo, we throw an error and ask you to use --force flag.

WDYT?

@omesser
Copy link
Contributor Author

omesser commented Mar 16, 2022

"Do you think it's fine to support them? I mean, people will rely on them and if we decide to remove some of them in future"

I believe it's a better experience to support the aliases. But we can still be consistent with the syntax in the docs (meaning always using "registrations" everywhere, and mention the aliases only once).
We will need to, indeed, not remove those in the future - or at least manage deprecations properly.
Another, "safer" route, is to explicitly mention in the docs that we only commit to a specific form of the arg = "registration", and not aliases like "reg" "registrations". I think both are valid approaches, but I think the aliases route has proved itself so convenient in some cases that it should be regarded as a production feature (just my personal 2 cents)

@omesser
Copy link
Contributor Author

omesser commented Mar 16, 2022

"In fact, model and models may be different things...."

This is why i originally wrote that this is debatable, but I think pluralization of types shouldn't be so close to a possible typo (like model and models).
Type should always be singular, if we ever want to deal with a set of models as a distinct type, the new type can be called model-set (or list? depends) - then you both accommodate for a collection of items but prevent easily made typos

@aguschin aguschin added this to the First GTO release milestone Mar 21, 2022
@aguschin
Copy link
Contributor

Ok, so I've created a TYPE_ALLOWED setting in #87. I think it's a way to deal with these, and we could add something on top like asking Did you mean "model"? if user entered "models". But don't think it's needed right now. I'll close the issue and we'll address this in future if that would be needed. Thanks for posting the feedback @omesser!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants