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

Support --extensions=minimal_set & bug fix #13249

Merged
merged 6 commits into from
Feb 5, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Feb 2, 2018

Follow up to #13203
@rsimha-amp I didn't update --extensions=minimal_set to DEVELOPING.md because it was not there before. Also I don't know if we want to document a single special value to the --extensions flag. Let me know. Thanks!

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Nice! Some comments below.

Also, let's add documentation for --minimal_set to DEVELOPING.md, and make sure that gulp dist --fortesting is documented too.

gulpfile.js Outdated
@@ -723,8 +742,21 @@ function dist() {
* Copy built extension to alias extension
*/
function copyAliasExtensions() {
if (!! argv.noextensions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the !! here.

gulpfile.js Outdated
process.exit(1);
}
log(green('Building extension(s):'),
cyan(argv.extensions.split(',').join(', ')));
if (argv.extensions == 'minimal_set') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need === here to avoid type conversions.

gulpfile.js Outdated
} else {
log(green('Building extension(s):'),
cyan(argv.extensions.split(',').join(', ')));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify lines 642-651 to:

      if (argv.extensions === 'minimal_set') {
        argv.extensions =
            'amp-ad,amp-ad-network-adsense-impl,amp-audio,amp-video,' +
            'amp-image-lightbox,amp-lightbox,amp-sidebar,' +
            'amp-analytics,amp-app-banner';
      }
      log(green('Building extension(s):'),
          cyan(argv.extensions.split(',').join(', ')));

gulpfile.js Outdated
green('to choose which ones to build.'));
green('to choose which ones to build, or'),
cyan('--extensions=minimal_set'),
green('to build ones needed to load article.amp.html.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

green('to build the ones needed to load'), cyan('article.amp.html') + green('.'));

(added the and color formatting)

gulpfile.js Outdated
green('to choose which ones to build.'));
green('to choose which ones to build, or'),
cyan('--extensions=minimal_set'),
green('to build ones needed to load article.amp.html.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

green('to build the ones needed to load'), cyan('article.amp.html') + green('.'));

(added the and color formatting)

gulpfile.js Outdated
cyan('--extensions=amp-foo,amp-bar'),
green('to choose which ones to build, or'),
cyan('--extensions=minimal_set'),
green('to build ones needed to load article.amp.html.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

No green in error messages. Use:

        log(red('ERROR:'), 'Missing list of extensions. Expected format:',
            cyan('--extensions=amp-foo,amp-bar'),
            'to choose which ones to build, or',
            cyan('--extensions=minimal_set'),
            'to build the ones needed to load',
            cyan('article.amp.html') + '.');

@rsimha
Copy link
Contributor

rsimha commented Feb 3, 2018

I just realized that by moving all the --extensions parsing code into performBuild, you're disabling the --minimal_set behavior of gulp dist. I think it makes sense to refactor the elaborate argument parsing / mutating into a new function that can be reused by dist().

Could you first address my comments above in one commit so the diffs are easy to review, and then follow it up with one that enables --extensions for dist()?

@zhouyx
Copy link
Contributor Author

zhouyx commented Feb 3, 2018

Nice catch! Got it!
I'll add support to --extensions=minimal_set to gulp dist in separate commit.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Yay! LGTM with one comment.

gulpfile.js Outdated
* Prints a helpful message that lets the developer know how to build
* a list of extensions or without any extensions.
*/
function printExtensionsHelp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function does more than just print help messages. It also parses the extension flags. Mabye rename it to parseExtensionFlags()?

gulpfile.js Outdated
@@ -609,7 +609,7 @@ function printConfigHelp(command) {
* Prints a helpful message that lets the developer know how to build
Copy link
Contributor

Choose a reason for hiding this comment

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

Update function description too :)

@zhouyx zhouyx merged commit c20c36e into ampproject:master Feb 5, 2018
protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018
* fix dist with alias

* change minimal_set usage

* fix documentation

* refactor print logic

* rename

* update function description
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* fix dist with alias

* change minimal_set usage

* fix documentation

* refactor print logic

* rename

* update function description
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
* fix dist with alias

* change minimal_set usage

* fix documentation

* refactor print logic

* rename

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

Successfully merging this pull request may close these issues.

4 participants