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(aft): Docs commands #2965

Merged
merged 8 commits into from
Jun 28, 2023
Merged

feat(aft): Docs commands #2965

merged 8 commits into from
Jun 28, 2023

Conversation

dnys1
Copy link
Contributor

@dnys1 dnys1 commented May 2, 2023

Adds aft docs build and aft docs serve to compile docs and serve docs on a local HTTP server with auto-rebuild, respectively.

@dnys1 dnys1 requested a review from a team as a code owner May 2, 2023 18:32
#
# TODO(dnys1): Create mapping of Flutter->Dart versions so we can just use Dart
# vended by Flutter.
- name: Setup Flutter
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed for publishing the built docs with workflow at some point?

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. Even though amplify_core is Dart-only, the examples we showcase in its docs all reference Flutter packages. So the doc package in amplify_core needs a dependency on Flutter so that we can have Flutter-specific samples in the amplify_core docs.

HuiSF
HuiSF previously approved these changes May 2, 2023
packages/aft/lib/src/commands/docs_command.dart Outdated Show resolved Hide resolved
@fjnoyp
Copy link
Contributor

fjnoyp commented May 3, 2023

Would the next step be to automatically call your aft build docs command to generate docs for each release?

Is it possible for users navigating our Github to see the index.html of that package's doc output in the current package folder? So if they're navigating our code and want to see docs for datastore or aft, they can directly see the docs/index.html and get the information they need directly within AmplifyFlutter/packages/datastore?

It seems right now we consolidate all docs in the output dir.

@fjnoyp
Copy link
Contributor

fjnoyp commented May 3, 2023

I appear to be running the command wrong?

I was able to run aft docs build in aft and get a doc file generated, but when I tried running aft docs build at the packages or root level, I get the following output:

➜  packages git:(feat/aft/docs) ✗ aft docs build
Building docs for amplified_todo...
Building docs for infra...
Building docs for aft...
Building docs for amplify_flutter...

Which then hangs indefinitely. Looking at the code I'm not 100% sure why amplified_todo keeps getting selected as well. Regardless it seems I cannot run the build command for all of amplify? I've run aft clean and bootstrap. I see that the docs files are generated for aft/amplified_todo/infra.

@dnys1
Copy link
Contributor Author

dnys1 commented May 9, 2023

@fjnoyp

Would the next step be to automatically call your aft build docs command to generate docs for each release?

You mean to verify before publishing?

Maybe. I think a next step could be to build docs for PRs and publish them to a temporary domain similar to the docs repo so that changes are more visible.

Is it possible for users navigating our Github to see the index.html of that package's doc output in the current package folder? So if they're navigating our code and want to see docs for datastore or aft, they can directly see the docs/index.html and get the information they need directly within AmplifyFlutter/packages/datastore?

What would be the benefit of this over visiting the published docs on pub.dev?

I appear to be running the command wrong?

You are running the command correctly. I've pushed some updates to make the error output more clear.

Running aft docs build will run it for every package in the repo, though, which may take a while. I think the goal of these commands is to enable quick docs development, so running aft docs serve from a package directory will serve up that doc's packages and listen for changes which will auto-rebuild the docs.

aft docs build should probably only be used locally with an --include option which targets a specific subset of packages, unless you genuinely wish to see all the packages' docs.

Comment on lines +21 to +22
doc/api/
**/doc/api/
Copy link
Member

Choose a reason for hiding this comment

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

Do these not exclude the same dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No one is the top-level doc dir, the other applies to all sub-packages

@@ -5,4 +5,3 @@ analyzer:
public_member_api_docs: ignore
exclude:
- lib/**/*.g.dart
- doc/
Copy link
Member

Choose a reason for hiding this comment

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

Can you verify we don't lose any pub points from analysis by changing this? Looks like we're currently 130/140, but lets avoid a regression if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is ultimately to not require any excludes which will help us catch analysis errors in CI which may show up in pub. Removing this exclude will help make sure our pub points remain high 👍

fjnoyp
fjnoyp previously approved these changes Jun 28, 2023
Copy link
Contributor

@fjnoyp fjnoyp left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification there, I scoped the docs serve command to get a reasonable subset to run docs against. Looking good so approved.

Dillon Nys added 8 commits June 28, 2023 15:10
No longer needed since we don't embed `pub`
Adds `aft docs build` and `aft docs serve` to compile docs and serve docs on a local HTTP server with auto-rebuild, respectively.
Some Dart packages require Flutter to be available, for example `amplify_core` requires it for its `doc` package.
@dnys1 dnys1 merged commit af39275 into main Jun 28, 2023
@dnys1 dnys1 deleted the feat/aft/docs branch June 28, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants