Fix bug when short_name is not present name is not used for audit #350

wardpeet · 2016-05-23T19:37:34Z

Fixes #348

googlebot · 2016-05-23T19:37:36Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

wardpeet · 2016-05-23T19:38:58Z

I signed it!

googlebot · 2016-05-23T19:38:59Z

CLAs look good, thanks!

samccone · 2016-05-24T17:17:10Z

src/audits/manifest/short-name-length.js

@@ -50,14 +50,17 @@ class ManifestShortNameLength extends Audit {
    let debugString;
    const manifest = artifacts.manifest.value;
    const suggestedLength = 12;
+    const shortNameValue = manifest && manifest.short_name && manifest.short_name.value;


can we add helper fns here instead of depending on the && last value behavior in JS

would it be ok to use Conditional (ternary) Operator?

yep :) I still like just a function or something but a ternary is nicer than an && chain

will make functions :) will be more readable indeed

samccone · 2016-05-24T17:17:51Z

I like what is going on here, thanks @wardpeet :) Just a few comments ... Thanks!

paulirish · 2016-05-24T19:06:36Z

src/audits/manifest/short-name-length.js


-    if (manifest && manifest.short_name && manifest.short_name.value) {
+    if (manifestValue) {
      // Historically, Chrome recommended 12 chars as the maximum length to prevent truncation.


Can you add a comment here that explains that name is used if short_name not present (hopefully with a link to the docs/source on that?)

wardpeet · 2016-05-26T10:15:51Z

Will have an updated pr later today :)

wardpeet · 2016-05-26T15:46:46Z

As promised :)
One thing i don't like is that I duplicate _getManifestShortName & _getManifestName. I rather put them in src/lib/util/manifest-helpers.js (or something) and just export those functions.

Another option is appending a getter function to the manifest object manifest.getValue('short_name')

paulirish · 2016-05-28T02:24:11Z

It definitely gets pretty low signal:noise with this breakout.

I have a PR against ward's fork that tries out lodash.get.
It gives some nice brevity and i'm looking forward to using it elsewhere in the project.

wardpeet#1

wardpeet · 2016-05-29T11:29:11Z

Looks better, didn't knew i could use this resolvedName = shortNameValue || nameValue; because of @samccone comment on &&. Merging as we speak

googlebot · 2016-05-29T11:30:59Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

wardpeet · 2016-05-29T17:35:36Z

mmh somehow i get rejected but when i run it locally i have no issues.

samccone · 2016-05-30T23:14:18Z

Fixes #350 also

samccone · 2016-05-30T23:16:35Z

package.json

@@ -38,6 +38,7 @@
    "gl-matrix": "2.3.2",
    "handlebars": "^4.0.5",
    "jszip": "2.6.0",
+    "lodash.get": "^4.3.0",


I would prefer not adding a new dep and just using the previous approach for now. We can tackle this cleanup after

I would prefer not adding a new dep and just using the previous approach for now.

This was my doing. The fix was grossly verbose without it. :/

samccone · 2016-05-30T23:18:26Z

Hey @wardpeet thanks for the PR!!! 🍝

The failure is due to the closure compiler not knowing what _get is. For now as I said in the codereview let's just drop the new dependency (we can tackle that cleanup after we land this)

Thanks again for the PR, looking forward to landing this one!

brendankenny · 2016-05-31T07:22:43Z

src/audits/manifest/short-name.js

@@ -49,8 +50,11 @@ class ManifestShortName extends Audit {
    let hasShortName = false;
    const manifest = artifacts.manifest.value;

-    if (manifest && manifest.short_name) {
-      hasShortName = (!!manifest.short_name.value);
+    // putting a comment to test travis again


wardpeet · 2016-05-31T17:56:59Z

ok i got lost :p so should i go back to the helpers i created or just do the hasShortName = !!(manifest.short_name.value || manifest.name.value);

brendankenny · 2016-05-31T21:29:24Z

@wardpeet sorry, nothing like reviewers disagreeing with each other to make everything crystal clear :)

Unless others disagree, I believe my suggestion above is the way to go. It's not verbose, as @paulirish wanted to avoid, and it doesn't introduce a new dependency, as @samccone wanted to avoid. It's also correct for handling all the possible types the manifest parser could hand back, so you'll have covered all your bases there as well.

samccone · 2016-05-31T21:41:46Z

👍 sounds good @brendankenny

paulirish · 2016-06-01T00:55:46Z

Unless others disagree, I believe my suggestion above is the way to go

👍

googlebot · 2016-06-02T18:50:36Z

CLAs look good, thanks!

wardpeet · 2016-06-02T19:20:47Z

I used @brendankenny suggestion and it looks like it works now. I'm fixing some tests I deleted which I thought I didn't needed but i do :)

paulirish · 2016-06-02T19:35:29Z

lgtm. thanks for being a trooper on this, @wardpeet. we feel really bad we jerked you around. :/

brendankenny · 2016-06-02T19:39:29Z

src/audits/manifest/short-name-length.js

-      isShortNameShortEnough = (manifest.short_name.value.length <= suggestedLength);
+      // https://developer.chrome.com/apps/manifest/name#short_name
+      let manifestValue = manifest.short_name.value || manifest.name.value || '';
+      isShortNameShortEnough = manifestValue && manifestValue.length <= suggestedLength;


@paulirish no problem :) Glad I could be a part of the discussion :p

brendankenny · 2016-06-02T19:46:17Z

one last thing and then LGTM too

wardpeet · 2016-06-02T21:13:51Z

And it's added 👍

brendankenny · 2016-06-02T21:19:08Z

test/src/audits/manifest/short-name-length.js

+  it('fails when a manifest contains no short_name and no name', () => {
+    const inputs = JSON.stringify({
+      name: null,
+      short_name: null


should live in the short-name.js test since this isn't really testing name length

brendankenny · 2016-06-02T22:57:25Z

LGTM! Thanks for your patience

paullewis · 2016-06-03T08:25:07Z

Thanks from me, too... This was one lengthy PR tennis match!

wardpeet · 2016-06-03T08:30:28Z

No worries ;) glad I could help. And hey now I can say I got a PR from @paulirish :p

samccone reviewed May 24, 2016
View reviewed changes

samccone added the chillin ❄ label May 24, 2016

paulirish reviewed May 24, 2016
View reviewed changes

paulirish removed the chillin ❄ label May 25, 2016

wardpeet force-pushed the bug/fix-name-without-shortname branch from 66549d6 to 87572c4 Compare May 26, 2016 15:40

wardpeet force-pushed the bug/fix-name-without-shortname branch from 45ced7b to b8af8fe Compare May 29, 2016 12:45

addyosmani mentioned this pull request May 30, 2016

manifest's "short_name" isn't required if "name" is short enough #348

Closed

samccone reviewed May 30, 2016
View reviewed changes

brendankenny reviewed May 31, 2016
View reviewed changes

wardpeet force-pushed the bug/fix-name-without-shortname branch from b8af8fe to 348d5ef Compare June 2, 2016 18:50

wardpeet force-pushed the bug/fix-name-without-shortname branch from 348d5ef to b741197 Compare June 2, 2016 19:19

paulirish added the +1 label Jun 2, 2016

wardpeet force-pushed the bug/fix-name-without-shortname branch from b741197 to e3d4c11 Compare June 2, 2016 19:37

brendankenny reviewed Jun 2, 2016
View reviewed changes

wardpeet force-pushed the bug/fix-name-without-shortname branch from e3d4c11 to 3c873c0 Compare June 2, 2016 21:13

brendankenny reviewed Jun 2, 2016
View reviewed changes

Fix bug when short_name is not present name is not used for audit

4187668

wardpeet force-pushed the bug/fix-name-without-shortname branch from 3c873c0 to 4187668 Compare June 2, 2016 21:57

brendankenny added the +2 label Jun 2, 2016

brendankenny merged commit 644d55a into GoogleChrome:master Jun 2, 2016

wardpeet deleted the bug/fix-name-without-shortname branch April 29, 2017 10:18

Fix bug when short_name is not present name is not used for audit #350

Fix bug when short_name is not present name is not used for audit #350

Conversation

wardpeet commented May 23, 2016

googlebot commented May 23, 2016

wardpeet commented May 23, 2016

googlebot commented May 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samccone commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented May 26, 2016

wardpeet commented May 26, 2016 • edited

paulirish commented May 28, 2016

wardpeet commented May 29, 2016

googlebot commented May 29, 2016

wardpeet commented May 29, 2016

samccone commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samccone commented May 30, 2016

Choose a reason for hiding this comment

wardpeet commented May 31, 2016

brendankenny commented May 31, 2016

samccone commented May 31, 2016

paulirish commented Jun 1, 2016

googlebot commented Jun 2, 2016

wardpeet commented Jun 2, 2016 • edited

paulirish commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 2, 2016

wardpeet commented Jun 2, 2016

Choose a reason for hiding this comment

brendankenny commented Jun 2, 2016

paullewis commented Jun 3, 2016

wardpeet commented Jun 3, 2016

wardpeet commented May 26, 2016 •

edited

wardpeet commented Jun 2, 2016 •

edited