New: Add manifest-scoped-urls #2328

shivangg · 2019-04-29T15:58:07Z

Pull request checklist

Make sure you:

Signed the Contributor License Agreement
Followed the commit message guidelines

For non-trivial changes, please make sure you also:

Added/Updated related documentation.
Added/Updated related tests.

Short description of the change(s)

Fixes #2058

molant

Nice job, a few comments and requests for more tests but this is looking good.

packages/hint-manifest-scoped-urls/README.md

packages/hint-manifest-scoped-urls/src/meta.ts

packages/hint-manifest-scoped-urls/src/hint.ts

packages/hint-manifest-scoped-urls/tests/tests.ts

molant · 2019-04-29T23:12:47Z

packages/hint-manifest-scoped-urls/tests/tests.ts

@@ -0,0 +1,84 @@
+import { test } from '@hint/utils';


I think we should also test the local connector but I'm not sure how to check that things are correct because paths can be anything locally 🤔
@antross do you have any suggestion?

molant · 2019-04-29T23:14:38Z

@shivangg also need to add location information when it makes sense (basically all cases except when the property doesn't exist I believe). Don't forget to add it to the tests as well!

shivangg · 2019-05-01T20:27:00Z

@molant Added location information to reports as well as tests.

molant · 2019-05-01T21:33:21Z

@shivangg looks like tests are failing. Can you please take a look at the output in Azure Pipelines

shivangg · 2019-05-01T21:52:47Z

@molant Was failing a test case. Should pass now (Linux and windows).

Checks that start_url is accessible and in scope. Checks either name or short_name is specified Resolves webhintio#2058

molant · 2019-05-02T14:36:09Z

@shivangg looks like Linux failed. I'm re-running it see if there's any change.

molant · 2019-05-02T15:55:09Z

And now it's passing 🎉

molant · 2019-05-02T15:56:26Z

@antross we have the pending question of local connector. Maybe we should change the scope of this one to be web only for now?

antross

Looking good overall. Just a few pieces we still need to sort out.

packages/hint-manifest-scoped-urls/README.md

packages/hint-manifest-scoped-urls/src/meta.ts

packages/hint-manifest-scoped-urls/src/hint.ts

packages/hint-manifest-scoped-urls/tests/tests.ts

packages/hint-manifest-scoped-urls/README.md

packages/hint-manifest-scoped-urls/src/hint.ts

molant

Very minor things, I think that's all about strings. It's looking really good!

packages/hint-manifest-scoped-urls/README.md

packages/hint-manifest-scoped-urls/package.json

packages/hint-manifest-scoped-urls/src/hint.ts

antross

Looks like a few of the tests are failing in CI:

✖ [chrome] Manifest property start_url is relative and inaccessible. No reports match "Specified 'start_url' is not accessible. Status code: '404'." or its location ({"column":21,"line":2}).
✖ [chrome] No start_url property specified in Manifest file No reports match "Property 'start_url' not found in manifest file." or its location ({"column":-1,"line":-1}).
✖ [chrome] Manifest property start_url has preceding / and scoped but inaccessible No reports match "Specified 'start_url' is not accessible. Status code: '404'." or its location ({"column":21,"line":2}).

Otherwise just some minor cleanup suggestions. Once the tests are fixed this should be ready to merge.

packages/hint-manifest-scoped-urls/README.md

packages/hint-manifest-scoped-urls/src/hint.ts

As per code review suggestions Co-Authored-By: shivangg <shivangg@users.noreply.github.com>

shivangg · 2019-05-07T17:54:31Z

All tests passing now. 😄

antross

Looks good to me. Nice work @shivangg! 🎉

molant · 2019-05-07T20:25:14Z

/azp run

azure-pipelines · 2019-05-07T20:25:32Z

Azure Pipelines successfully started running 1 pipeline(s).

molant · 2019-05-07T20:25:45Z

@antross @shivangg now that we have code coverage back I'm going to run CI again to make sure we meet the threshold.

shivangg · 2019-05-07T22:22:32Z

@molant It passes the thresholds in the Azure pipelines.

shivangg requested review from antross, molant and sarvaje as code owners April 29, 2019 15:58

shivangg added the area:hint label Apr 29, 2019

shivangg force-pushed the feat/manifest-scoped-urls branch from dae489d to fcc0b12 Compare April 29, 2019 21:37

molant requested changes Apr 29, 2019

View reviewed changes

molant reviewed Apr 29, 2019

View reviewed changes

molant mentioned this pull request Apr 30, 2019

[Feature] Convert both Category and HintScope to const enum [0.1] #2335

Closed

shivangg force-pushed the feat/manifest-scoped-urls branch from 381d149 to 03b6198 Compare May 1, 2019 21:43

shivangg added 8 commits May 2, 2019 18:36

New: Add manifest-scoped-urls

646f57f

Checks that start_url is accessible and in scope. Checks either name or short_name is specified Resolves webhintio#2058

Chore: Add tests for hint-manifest-scoped-urls

874264f

Docs: Add examples for manifest-scoped-urls usage

0dbe232

Chore: Minor refactoring

0c7d211

New: Add problemLocation to reports

be2ae8c

Chore: Minor refactoring

ed092f9

New: Report if start_url not same origin

5fda78f

Chore: Add tests to cover more scenarios

807e464

shivangg force-pushed the feat/manifest-scoped-urls branch from 03b6198 to 807e464 Compare May 2, 2019 13:07

shivangg requested a review from molant May 2, 2019 13:56

shivangg added this to In progress in Pesto Apprenticeship May 2, 2019

antross requested changes May 2, 2019

View reviewed changes

This was referenced May 2, 2019

[Feature] Make hint-manifest-scoped-urls work with local connector #2360

Open

[Bug] Scope hint-manifest-icons to site #2361

Closed

shivangg force-pushed the feat/manifest-scoped-urls branch from 97ec5c4 to 4c5499d Compare May 2, 2019 20:49

shivangg requested a review from antross May 2, 2019 22:51

molant mentioned this pull request May 3, 2019

Enable scope.any in manifest-icons and manifest-scoped-urls #2363

Open

molant requested changes May 3, 2019

View reviewed changes

antross requested changes May 6, 2019

View reviewed changes

Chore: Minor refactoring

d8ce622

As per code review suggestions Co-Authored-By: shivangg <shivangg@users.noreply.github.com>

shivangg force-pushed the feat/manifest-scoped-urls branch from 62357cf to d8ce622 Compare May 6, 2019 18:06

shivangg requested review from antross and molant May 7, 2019 17:49

antross approved these changes May 7, 2019

View reviewed changes

molant approved these changes May 7, 2019

View reviewed changes

molant approved these changes May 8, 2019

View reviewed changes

molant merged commit d492bee into webhintio:master May 8, 2019

Pesto Apprenticeship automation moved this from In progress to Done May 8, 2019

shivangg deleted the feat/manifest-scoped-urls branch May 8, 2019 04:01

shivangg self-assigned this May 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Add manifest-scoped-urls #2328

New: Add manifest-scoped-urls #2328

shivangg commented Apr 29, 2019

molant left a comment

molant Apr 29, 2019

molant commented Apr 29, 2019

shivangg commented May 1, 2019

molant commented May 1, 2019

shivangg commented May 1, 2019

molant commented May 2, 2019

molant commented May 2, 2019

molant commented May 2, 2019

antross left a comment

molant left a comment

antross left a comment

shivangg commented May 7, 2019

antross left a comment

molant commented May 7, 2019

azure-pipelines bot commented May 7, 2019

molant commented May 7, 2019

shivangg commented May 7, 2019

New: Add manifest-scoped-urls #2328

New: Add manifest-scoped-urls #2328

Conversation

shivangg commented Apr 29, 2019

Pull request checklist

Short description of the change(s)

molant left a comment

Choose a reason for hiding this comment

molant Apr 29, 2019

Choose a reason for hiding this comment

molant commented Apr 29, 2019

shivangg commented May 1, 2019

molant commented May 1, 2019

shivangg commented May 1, 2019

molant commented May 2, 2019

molant commented May 2, 2019

molant commented May 2, 2019

antross left a comment

Choose a reason for hiding this comment

molant left a comment

Choose a reason for hiding this comment

antross left a comment

Choose a reason for hiding this comment

shivangg commented May 7, 2019

antross left a comment

Choose a reason for hiding this comment

molant commented May 7, 2019

azure-pipelines bot commented May 7, 2019

molant commented May 7, 2019

shivangg commented May 7, 2019