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

CADL APIView filters out things in the global namespace #5289

Closed
catalinaperalta opened this issue Jan 31, 2023 · 17 comments · Fixed by #5378
Closed

CADL APIView filters out things in the global namespace #5289

catalinaperalta opened this issue Jan 31, 2023 · 17 comments · Fixed by #5378
Assignees
Labels
APIView TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl)

Comments

@catalinaperalta
Copy link
Member

Can we add the CADL DPG modifications that are defined in client.cadl?

Example CADL that did not include client.cadl definitions: https://apiview.dev/Assemblies/Review/205e1a393e0c4ed8b5a42fb5ba5988cd

PR for the CADL API view shared above: https://github.com/Azure/azure-rest-api-specs-pr/pull/10411

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 31, 2023
@catalinaperalta
Copy link
Member Author

cc @maririos @praveenkuttappan

@github-project-automation github-project-automation bot moved this to 🆕 New in ApiView Jan 31, 2023
@praveenkuttappan praveenkuttappan added the TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl) label Jan 31, 2023
@tjprescott
Copy link
Member

@catalinaperalta you can already do this by generating a token file manually (likely not by using a URL).

Since this pertains to the automatically generated APIView, this would fall to Central EngSys.

@tjprescott tjprescott added the Central-EngSys This issue is owned by the Engineering System team. label Jan 31, 2023
@maririos maririos removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 1, 2023
@maririos
Copy link
Member

maririos commented Feb 1, 2023

@tjprescott you mean that the automatically generated APIView from that PR is not correct? what it is missing?

@tjprescott
Copy link
Member

@maririos the automatic PR is only targeting main.cadl. @catalinaperalta wants it to target client.cadl (which will pull in main.cadl) so that she sees the effect of the sidecar file.

However... for automatic PRs, it doesn't really make sense to apply the sidecar. It's supposed to a be a view of the service, not the client. So perhaps in this use case, the only appropriate course of action is to make a manual APIView targeting the client.cadl. In theory this could be done with the URL-based manual review as well by pointing to the client.cadl file.

@tjprescott
Copy link
Member

So assuming that the manual PR where you target a URL works by targeting a client.cadl file, I think this issue could actually be closed.

@catalinaperalta
Copy link
Member Author

@tjprescott I manually generated to token file for the CADL APIview and uploaded that one. I did not use the URL since I wasnt sure what URL was expected. So should the manual token file have showcased the client.cadl? I did not see it in the API view I uploaded.

@catalinaperalta
Copy link
Member Author

Maybe I'm on an older version of the package? This is what I'm using:

"@azure-tools/cadl-apiview": "^0.3.4",

@tjprescott
Copy link
Member

The manual token file (assuming you targeted client.cadl) should include all of your augment decorators.

@maririos
Copy link
Member

maririos commented Feb 2, 2023

@praveenkuttappan could you help with this?

@tjprescott I manually generated to token file for the CADL APIview and uploaded that one. I did not use the URL since I wasnt sure what URL was expected. So should the manual token file have showcased the client.cadl? I did not see it in the API view I uploaded.

More on issue: #5273

@praveenkuttappan
Copy link
Member

Baedon the above message, @catalinaperalta is asking about manual review gen locally by running the parser and so this should be something @tjprescott needs to check the parser and help.

@tjprescott
Copy link
Member

tjprescott commented Feb 2, 2023

@praveenkuttappan targeting a client.cadl file by running the parser locally already works. The question is whether it works for the URL-based manual APIView.

Even though the issue requested it, I don't think it makes any sense for automatic APIViews to target client.cadl (or to even enable that).

@tjprescott
Copy link
Member

@tjprescott I manually generated to token file for the CADL APIview and uploaded that one. I did not use the URL since I wasnt sure what URL was expected. So should the manual token file have showcased the client.cadl? I did not see it in the API view I uploaded.

@catalinaperalta what is the link to the APIView generated and what command did you use to generate it? It should look something like: cadl compile client.cadl --emit=@azure-tools/cadl-apiview.

@praveenkuttappan
Copy link
Member

@praveenkuttappan targeting a client.cadl file by running the parser locally already works. The question is whether it works for the URL-based manual APIView.

Even though the issue requested it, I don't think it makes any sense for automatic APIViews to target client.cadl (or to even enable that).

So I assume we don't need any change in central engsys side. We have another issue to track improving guidelines on how to create API review for CADL.

@praveenkuttappan praveenkuttappan moved this from 🆕 New to 📋 Backlog in ApiView Feb 6, 2023
@catalinaperalta
Copy link
Member Author

Sorry for the delay @tjprescott, I did use exactly that command. I also just tried it again with the same result.

Command: cadl compile .\client.cadl --emit="@azure-tools/cadl-apiview"
APIview: https://apiview.dev/Assemblies/Review/3d80f1b45fec4bc4b97a39c991948d79

@tjprescott
Copy link
Member

tjprescott commented Feb 7, 2023

@catalinaperalta this happens because your client declarations are not inside a namespace:
https://apiview.dev/Assemblies/Review/d35ab17a167648f29dcb5ee8cae98062

@catalinaperalta
Copy link
Member Author

catalinaperalta commented Feb 7, 2023

I see. Is the namespace required for the CADL Azure DPG declarations? If so, I think we may need to document this somewhere and/or have it as a linter rule. Thanks for looking into this @tjprescott!

@tjprescott
Copy link
Member

Well, if you put it any namespace starting with HealthDecisionsApi it will render. That is the APIView logic because otherwise it will render literally everything under the sun from Cadl, Azure.Core, etc. However the logic is flawed because it is supposed to support your package namespace and subnamespaces, but in this case you can have a package namespace of Foo and it will happily accept Foo2 which isn't a subnamespace at all. So there is some logic for me to revisit (especially since it relies on now-deprecated stuff). Whether to render things in the global namespace... I'm not sure.

Also, I opened this issue (microsoft/typespec#1625) that I ran into while looking into this. But for now you should be unblocked.

@tjprescott tjprescott removed the Central-EngSys This issue is owned by the Engineering System team. label Feb 7, 2023
@tjprescott tjprescott changed the title [Feature request][bug] CADL API view is now showing DPG definitions in client.cadl CADL APIView filters out things in the global namespace Feb 7, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in ApiView Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIView TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl)
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants