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

Add --generate-api-docs to build script #17057

Merged
merged 3 commits into from Jun 1, 2018

Conversation

Projects
None yet
2 participants
@Arcanemagus
Contributor

Arcanemagus commented Mar 30, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Add a new option --generate-api-docs that changes the build script to only do what is required to build the API documentation, skipping the process of building the full Atom binaries.

Alternate Designs

Continuing to hack the build scripts manually to follow this behavior whenever a documentation only change is needed.

Why Should This Be In Core?

It is an enhancement for the build script already in core.

Benefits

Adds the ability to only build the documentation when testing changes that only affect that and don't need the full binary to be built.

Possible Drawbacks

Slightly higher complexity to the build script.

Verification Process

Tested that this only does the necessary steps to build the documentation.

Applicable Issues

None.

Add --generate-api-docs to build script
Add a new option `--generate-api-docs` that changes the build script to
only do what is required to build the API documentation, skipping the
process of building the full Atom binaries.
@daviwil

Thanks for adding the new argument, this will be handy! Just a couple comments here, nothing major

.describe('install', 'Install Atom')
.string('install')
.wrap(yargs.terminalWidth())
.argv
// Needed so we can require src/module-cache.coffee during generateModuleCache

This comment has been minimized.

@daviwil

daviwil May 31, 2018

Member

Moving this just because it isn't used in the previous section or was there another benefit?

This comment has been minimized.

@Arcanemagus

Arcanemagus May 31, 2018

Contributor

I think this is a leftover from a separate change where I was trying to get rid of all the dependency requires (to exit early when --help is called), however as the arguments are parsed in a dependency, it doesn't actually make that much of a difference. I can remove this change if you would prefer.

This comment has been minimized.

@daviwil

daviwil May 31, 2018

Member

Removing it would be ideal, thanks a bunch!

}
if (argv.generateApiDocs) {
process.exit()

This comment has been minimized.

@daviwil

daviwil May 31, 2018

Member

I think you'll want to wait for the promise to complete before exiting, just in case. Maybe it's better to add another conditional around the block of code below to check for the presence of that flag before adding the .then()s to binariesPromise? That way the process will exit on its own after the promise finishes if we're only generating API docs.

This comment has been minimized.

@Arcanemagus

Arcanemagus May 31, 2018

Contributor

As at this stage binariesPromise is just a reference to Promise.resolve() that hasn't been called yet, there are no "in-flight" promises to wait for a resolution of as all of the above code is synchronous.

That being said, wrapping the entire block below in a check of that flag is the more correct way of doing this if you want me to do that.

This comment has been minimized.

@daviwil

daviwil May 31, 2018

Member

Yep, using the if statement might make it a bit more clear that the promise isn't just being ignored. binariesPromise gets assigned from dumpSymbols if --existing-binaries isn't passed, so there will actually be a pending promise at that point.

Edit: I'm wrong, there isn't a pending promise :) Was looking at the old version of the code.

Arcanemagus added some commits Jun 1, 2018

Revert moving of dependencies
This change isn't necessary for adding the option.
Wrap binariesPromise handling in an if
Instead of forcing an immediate exit with process.exit(), wrap the
handling of binariesPromise in an `if` statement, allowing the script to
exit on its own if passed the new option.
@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Jun 1, 2018

@daviwil Updated, thanks for looking over this!

@daviwil

daviwil approved these changes Jun 1, 2018

Sweet, thanks a lot for making the changes!

@Arcanemagus Arcanemagus merged commit 064fcce into master Jun 1, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Arcanemagus Arcanemagus deleted the la/generate-api-docs branch Jun 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment