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

CB-14148 remove nonsense "www" platform from Cordova listing #620

Merged
merged 4 commits into from Aug 15, 2018

Conversation

Projects
None yet
3 participants
@brodybits
Contributor

brodybits commented Jun 26, 2018

Platforms affected

All

What does this PR do?

  • Remove nonsense "www" platform from listing in cordova platform ls
  • Verify proper attributes and behavior of platforms object

What testing has been done on this change?

  • Verified that the nonsense "www" member is removed in spec
  • Verified working properly in local copy of cordova-lib and cordova-cli projects
  • Verified that npm test completely passes (in CI)

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.
@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Jun 27, 2018

LGTM, though I'd feel better knowing why this was in there in the first place. Could not find out anything about it using blame. 😒

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Jul 12, 2018

@dpogue @stevengill do you have any idea what the www platform's purpose is or was?

@dpogue

This comment has been minimized.

Member

dpogue commented Jul 12, 2018

I do not 😞

@raphinesse raphinesse requested a review from stevengill Jul 26, 2018

it('should *not* include nonsense "www" platform (CB-14148)', function () {
expect(platforms.www).not.toBeDefined();
});

This comment has been minimized.

@raphinesse

raphinesse Aug 15, 2018

Contributor

I've been battling needlessly verbose tests. Would you be OK if we merge the last three tests to the following?

it('should have all and only the supported platforms', function () {
    expect(Object.keys(platforms)).toEqual(jasmine.arrayWithExactContents([
        'android', 'browser', 'ios', 'osx', 'windows'
    ]));
});

With that change, I'd approve this PR. I think we should just risk removing the www platform.

This comment has been minimized.

@brodybits

brodybits Aug 15, 2018

Contributor

👍

This comment has been minimized.

@raphinesse

raphinesse Aug 15, 2018

Contributor

Do you want to incorporate that change?

This comment has been minimized.

@brodybits

brodybits Aug 15, 2018

Contributor

Yes I can incorporate the change later today. I would also like this PR to land with a commit message that will indicate what we did to the user in the RELEASENOTES, kinda like the way I updated the title.

@brodybits brodybits changed the title from CB-14148 remove nonsense "www" platform from listing to CB-14148 remove nonsense "www" platform from Cordova listing Aug 15, 2018

@brodybits brodybits removed the request for review from stevengill Aug 15, 2018

@brodybits brodybits merged commit bd92683 into apache:master Aug 15, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@brodybits brodybits deleted the brodybits:cb-14148-remove-nonsense-www-platform branch Aug 15, 2018

@brodybits

This comment has been minimized.

Contributor

brodybits commented Aug 15, 2018

Now merged with test cases merged as proposed by @raphinesse

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