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

feat: add source declaration links to API Explorer #578

Merged
merged 13 commits into from
Apr 13, 2021
Merged

Conversation

jkaster
Copy link
Contributor

@jkaster jkaster commented Apr 12, 2021

If a declarations index file produced with yarn mine is found, declaration source links get displayed for methods and types

The apix-files folder was added to provide an http server to serve the index files from the current source.

Run yarn serve from apix-files to serves the mined index files at http://localhost:30000.

@google-cla google-cla bot added the cla: yes label Apr 12, 2021
@jkaster jkaster changed the title feat:Added source declaration links to API Explorer feat: Added source declaration links to API Explorer Apr 12, 2021
@jkaster jkaster changed the title feat: Added source declaration links to API Explorer feat: added source declaration links to API Explorer Apr 12, 2021
@jkaster jkaster changed the title feat: added source declaration links to API Explorer feat: add source declaration links to API Explorer Apr 12, 2021
@joeldodge79
Copy link
Contributor

Looking at the source code changes I think the commit message (aka changelog entry) is fine but just wanted to re-iterate that the exact same changelog entry will show up in each of packages/{api-explorer,extension-api-explorer,sdk-codegen-scripts,sdk-codegen}/CHANGELOG.md given that files in all of those packages are are touched in this PR. Again, I think that makes sense given the file changes but just wanted to call it out in case you hadn't realized and feel any different

@@ -0,0 +1,38 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to house this under packages/? I realize there are no dependencies in either direction but still kind of seems to me like maybe we should keep all our typescript sdk-codegen packages under packages/ (and who knows, maybe there will be a dependency some day?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is not a monorepo package and doesn't need all the baggage associated with that. If it ever becomes something we customize, we'll move it into packages/apix-files. Currently, it's basically just a node script that needs to be isolated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, all those places were changed to support this so the CHANGELOG entry is appropriate.

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks okay.

Some minor doc issues.

Some things to think about

@@ -0,0 +1,40 @@
# API Explorer file server

API Explorer (aka APIX) can be uses some JSON-formatted "index" files to augment the information provided in a specification.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API Explorer (aka APIX) can be uses some

?

API Explorer (aka APIX) uses some

instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's definitely wrong now. I'll fix.


This will start the API Explorer file server at `http://localhost:30000`

## Mining the mother lode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any issues with mother lode as a politically correct term?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't *think" that's a charged term. It's taken from mining but I guess we can change it to "examples index" and "examplesIndex.json" to be less obscure

@@ -28,7 +28,7 @@ import { TabList, Tab, TabPanels, TabPanel, useTabs } from '@looker/components'
import { findExampleLanguages, IMethod } from '@looker/sdk-codegen'

import { CollapserCard } from '../Collapser'
import { LodeContext } from '../../context/examples'
import { LodeContext } from '../../context/motherlode'
import { DocExamples } from './DocExamples'

interface DocSdkUsageProps {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this as DocSausageProps :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely leaving it as is then!

oauth2_urls: []
}
}
```
the `http://localhost:3000` is for when you want to use the [API Explorer file server](/apix-files/README.md)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be interesting to see how this works in marketplace. Do we want to put a dummy entry in so that user can configure hosts to point at it? Or just go with localhost:3000. 3000 pretty popular. Consider using non popular port

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, documentation error. That's supposed to be 30000

- rename some things
- fix some documentation errors
@jkaster jkaster requested a review from bryans99 April 12, 2021 21:39
@github-actions

This comment has been minimized.

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkaster jkaster merged commit ce0e588 into main Apr 13, 2021
@jkaster jkaster deleted the jax/declaration-links branch April 13, 2021 00:43
@github-actions
Copy link
Contributor

Test Results

    3 files    18 suites   1m 5s ⏱️
  58 tests   37 ✔️ 12 💤   9 ❌
174 runs  111 ✔️ 36 💤 27 ❌

For more details on these failures, see this check.

Results for commit ce0e588.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants