Coder Social home page Coder Social logo

Comments (27)

Souvikns avatar Souvikns commented on July 28, 2024 2

you can differentiate, we just assume that file path is valid only if points to file with extension (. is there). And we can always do fallback, check first context and then file, or file and then context

Just as @derberg said we are following some fallback rules to automatically pick the async API file.

  • If current context is present then asyncapi validate automatically picks that.
  • If current context is not present and there is a asyncapi.yml or asyncapi.json or asyncapi.yaml file present in the working directory then asyncapi validate picks that file automatically.
  • If the user wants to pass in any other context then it passes from --context flag to pass in the context parameter.

As of now we are accepting the file path through --file flag. I think we can accept the file path as a input rather than a flag. It feels more natural, also generator uses Usage: ag [options] <asyncapi> <template> so any users coming from this CLI will have it easy to understand.

from cli.

derberg avatar derberg commented on July 28, 2024 1

maybe open another issue for the context topic as this issue is purely for functionality to support fetching spec from remote and not context behaviour itself

from cli.

magicmatatjahu avatar magicmatatjahu commented on July 28, 2024 1

We merged PR with --file so quickly, my fault 😅 Probably we should rethinking the way of passing doc to the CLI.

Currently we have feature called context, where we persist the (currently only local file) document of AsyncAPI and we can perform action on that. However, overriding current doc for the given command should be also available, and by this we have --file flag. Passing the file path/url/content of doc as argument (not as flag) like in our generator is very handy. We must also have in mind that in the future we will handle command for the multiple files, like asyncapi diff to check difference between two specs. And here is a problem: how to handle context and the another spec given as input? Context should persist only one spec, so the we will end probably with:

asyncapi diff <file-to-doc>

and then doc on which we will perform diff will be fetched from current spec...

So maybe going with flags, we should extend idea from @fmvilas and treat arguments as doc(s) using the format {context|doc}:{path|url|value} so we will end with:

  • using current context:

    asyncapi validate
  • using doc from given context:

    asyncapi validate context:custom-context
  • using doc from file/url (doc: we can omit):

    asyncapi validate doc:./path/to/file.yaml
  • using value as input

    asyncapi validate doc:${curl ...}
  • using multiple arguments (perform diff on doc from context and from file):

    asyncapi diff context:custom-context doc:./path/to/file.yaml

We can make this also by flags, but in this solution we treat doc as primary citizen of our CLI.

from cli.

derberg avatar derberg commented on July 28, 2024 1

@magicmatatjahu we had initial discusison on CLI here #1 and idea where AsyncAPI document is in the center got "dropped" for now. Btw, I also do not think approach with : us too intuitive.

btw, I closed this issue because of I'm closing this one, it is a bit outdated looking at what we already have better have new issue as the description is really super old and not matching current state of CLI, only confuses others.

file is a file, no matter if on local or remote, so no matter if ./asyncapi.yml or http://my.com/asyncapi.yml, these are still files. And it is a flag as it is optional, not mandatory, as you can have default context set + CLI looks for standard files names in CWD.

from cli.

magicmatatjahu avatar magicmatatjahu commented on July 28, 2024 1

@derberg Ok, I agree with your point of view, however my first thought is that --file points to the local file, not remote, so for that I suggested --spec or --doc.

And it is a flag as it is optional, not mandatory, as you can have default context set

Exactly, I know about that but I also wanted to put myself in the user's shoes and see if --file has a good UX and check if standalone arguments would be better.

from cli.

derberg avatar derberg commented on July 28, 2024 1

I agree with @fmvilas that spec is not a good word as we do not talk about spec here. And doc is not bad, I just think file is pretty neutral. When we talk about AsyncAPIs created by users, we talk about files and documents, but I think more common is talking about AsyncAPI file, as document always has this close relation to documentation, and we know AsyncAPI is more than that. I suggest we stay with file and in future I don't see a problem we add multiple names for the same option if larger group of users is confused

from cli.

derberg avatar derberg commented on July 28, 2024 1

yup, in the end, CLI is still not at 1.0 release, we are breaking commands and options all the time now 😄

from cli.

github-actions avatar github-actions commented on July 28, 2024

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

Here I would like to clarify something,

When the user sets the context he then does not have to pass in the async API spec path for other commands, so context is like a global setting for the CLI.
So my question is when the user sets the context how do we persist it? If we store the spec path or the URL in the local file system then when happens when the CLI is installed globally and the user is using it for multiple projects.

from cli.

derberg avatar derberg commented on July 28, 2024

context should be optional, and I still should be able to pass context with a flag in case I'm doing some automation, or working with multiple files.

context should also be smart in the way that if I do not have context set, and I do not pass --context command, CLI check if there are files called asyncapi.json/yml/yaml in workdir, and picks them up.

context should most probably be persisted in a file.

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

we can persist a file to context based on the directory the set context command was called, so basically {"process.cwd()": "async API spec path or URL"}

context should also be smart in the way that if I do not have context set, and I do not pass --context command, CLI check if there are files called asyncapi.json/yml/yaml in workdir, and picks them up.

Yeah if context is set then we by default that path or else id --context flag is passed then we use that context

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

Will it help if CLI parses both the local file and the URL and provides the parsed spec object for other tools to use.

from cli.

derberg avatar derberg commented on July 28, 2024

I'm closing this one, it is a bit outdated looking at what we already have better have new issue

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

So I read how this is implemented in generator. As of now, we are storing the local path to the spec in the context from where we are loading the current context as a result we do not need to pass the file path every time as an argument.

So to support the loading spec file from the URL, we have to consider two options.

  • --url parameter, and also update the useSpecFile hook that will be used by other commands in the future.
  • Check and persist URL as a context value, For this we need to update the contextService and also SpecificationFile class. But the question that arises should we persist URL in context because if we check an URL and it is working fine but in the future, the URL might not work as intended and then that stored context is wrong.

what are your thoughts on this @derberg @magicmatatjahu

from cli.

magicmatatjahu avatar magicmatatjahu commented on July 28, 2024

@Souvikns Rather than adding additional flag like --url, I suggest that we should change the --file flag to the --spec flag which will support both file path and url - CLI is in the development time, so we can make breaking changes, don't worry. Better now, not in the future.

About your concerns:

Check and persist URL as a context value, For this we need to update the contextService and also SpecificationFile class.

SpecificationFile implements this interface, so for url, we should create separate class SpecificationUrl.

But the question that arises should we persist URL in context because if we check an URL and it is working fine but in the future, the URL might not work as intended and then that stored context is wrong

In this same way, you can change the path of the file from current context and context won't work, so I think that we can save the URL in the contexts.

I'm wondering if we should download the spec from URL every time or just once and add a flag for re-downloading. We can have a discussion about that.

For me we should go for the --spec flag, but we can wait for the Łukasz, because there should be more that one voice - he will be back in the Monday :)

Whichever flag we choose, we should have in mind that someone would like to define options for downloading, perhaps similar to those in curl, or (which is better) allow passing as --spec also normal string, so then someone can have possibility to concatenate curl with our cli like:

asyncapi validate --spec ${curl ...}

so we will use the output from curl. I only have problem, if should we save also the stringified spec in the context 🤔 What do you think? :)

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

I've seen the discussion in #1 and gonna reply here so we move the conversation to this issue. I'd not use the --spec option because the "spec" is AsyncAPI and the file is a document. I think there's a base problem with this approach we're taking on the CLI. We're "abusing" options (those starting with -- or -) for mandatory things. When something is mandatory, it shouldn't be an option because, as the name implies, they're optional.

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

So I think it should be something like:

asyncapi validate <asyncapi-file-or-url>

It's easy to detect if the argument asyncapi-file-or-url is a URL or a file.

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

@fmvilas after persisting the context, we are able to intelligently pick up the spec path according to the current set context or by autodetecting any spec file from the working directory so asyncapi validate will automatically pick up the spec file and validate. So thus I was using the flag to override the initial choice.
Then if we have specfile path or url as input then should still keep the context as a flag or take that as an input as well. context-key-file-path-or-url.

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

Alright, I see what you mean. I think this "context" thing is gonna bite us hard in the future 😄 But it's not related to this issue so nevermind.

from cli.

Souvikns avatar Souvikns commented on July 28, 2024

@derberg I am starting with this issue next.

from cli.

derberg avatar derberg commented on July 28, 2024

not sure why now but I just got a second thought on --file and --context. AsyncAPI is the main resource here, not an option right. Flag === option? so we probably should do:

  • asyncapi validate /path/asyncapi.yml
  • asyncapi validate http://domain.com/asyncapi.yml
  • asyncapi validate my_context

I'm kinda with thoughts that @fmvilas had earlier. I think it is also related to #37

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

Heads up there will be no way to differentiate my_context from /path/asyncapi.yml. They both are valid paths. If we're going to rely on contexts, then the file/url should be optional, although not necessarily a flag. For instance:

asyncapi validate /path/asyncapi.yml
asyncapi validate http://domain.com/asyncapi.yml
asyncapi validate # Uses current context
asyncapi validate --context=my_context # Uses my_context context

from cli.

derberg avatar derberg commented on July 28, 2024

you can differentiate, we just assume that file path is valid only if points to file with extension (. is there). And we can always do fallback, check first context and then file, or file and then context

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

🤔 I'm not sure I get it. I still see a clash between path and context name:

asyncapi validate my_app

Is my_app a context name or a file path? What if it's an existing file but the user really wanted to use the my_app context?

you can differentiate, we just assume that file path is valid only if points to file with extension (. is there)

What prevents someone from using a context name with a dot (.)?

And we can always do fallback, check first context and then file, or file and then context

Yeah, I was assuming we do some fallback but the problem persists when the provided name is both an existing context name and a file.

from cli.

derberg avatar derberg commented on July 28, 2024

First of all what is the probability that someone has a context with dot in a name, and that the context name is my_app.yml. So the risk is there but imho it is ok to accept that risk in favour of better DX. With a proper fallback strategy, it will work fine. We can have -- debug flag that can log more info on what is exactly happening and what was picked up. But as a response from validation and in future other commands you always get a path to the file that was processed, so easy to pick by the user what was used.

from cli.

fmvilas avatar fmvilas commented on July 28, 2024

Alright, let's accept this theoretically low risk and learn from real usage data 👍

from cli.

asyncapi-bot avatar asyncapi-bot commented on July 28, 2024

🎉 This issue has been resolved in version 0.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

from cli.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.