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

Enhanced the report issue link derivation to respect the package.json bugs property #1093

Merged
merged 6 commits into from Apr 10, 2019

Conversation

Projects
None yet
3 participants
@maralv
Copy link
Contributor

commented Nov 20, 2018

The existing implementation ignores the package.json bugs property, when deriving the report issue link. The proposed solution checks if the bugs property is a string or an object, if it's an object it's parsed for an URL or email, according to the NPM package specification. The repository issue URL is used as fallback.

Requirements

None.

Description of the Change

Replaces the old getRepositoryUrl with a new getRepositoryBugUri that respects the bugs property in package.json.

Alternate Designs

None.

Benefits

For Atom packages using an issue tracker outside of GitHub or an email, the Report Issue button will now link to the correct place.

report-issue-button

Possible Drawbacks

As the proposed solution uses the old solution as fallback, there's no identified drawbacks.

Applicable Issues

#966
#1040

Enhanced the report issue link derivation to care about the package.json
bugs field, according to the NPM package specification. The respository
default issue url is used as fallback.
@rsese

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Thanks @maralv! Can you add tests for this?

@rsese rsese added the needs-testing label Nov 27, 2018

@maralv

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

I can surely give it a try! :)

@maralv

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Succeeded Tests

🐛 Please review the minor fix in commit 37445ba, it seems to be an old issue.
The added tests cover all new code and pass successfully.

@rsese

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Thank you! Someone from the team will take a look as soon as they can.

@rsese rsese added triaged and removed needs-testing labels Dec 4, 2018

@maralv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Thank you! Someone from the team will take a look as soon as they can.

Would appreciate a review, thank you!

@nathansobo nathansobo self-assigned this Apr 8, 2019

@nathansobo nathansobo merged commit 0a3afcf into atom:master Apr 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.