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

Fix #331 overrideUrl doesn't work #333

Merged
merged 1 commit into from May 12, 2019

Conversation

@digulla
Copy link

@digulla digulla commented May 10, 2019

In addition to just checking the result, I've also added logging so users of the plugin can find out what it's loading and from where and why without having to start a debugger.

Code coverage is at 83.9%

Sore point is the DEFAULT_OVERRIDE_THIRD_PARTY which is used in the default path of the code; I could wrap the constant in a getter to switch between "exists" and "missing" cases.

Copy link
Contributor

@ppalaga ppalaga left a comment

Except for the few minor things inline, please fix the issues reported by the CI.

Please squash all your commits into one and reword to Fix #331 overrideUrl doesn't work

@digulla
Copy link
Author

@digulla digulla commented May 10, 2019

Just to make sure: When you ask me to "squash the commit", I do that locally and then force-push, right?

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented May 10, 2019

Just to make sure: When you ask me to "squash the commit", I do that locally and then force-push, right?

Yes.

@digulla digulla force-pushed the bugfix/331-overrideUrl_doesnt_work branch from dd21738 to cd63928 May 10, 2019
@digulla
Copy link
Author

@digulla digulla commented May 10, 2019

I'm wondering why this issue is still in the state "Changes requested". Did I miss something?

@ppalaga ppalaga changed the title Bugfix/331 override url doesnt work Fix #331 overrideUrl doesn't work May 12, 2019
@ppalaga ppalaga merged commit c4015c1 into mojohaus:master May 12, 2019
2 checks passed
@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented May 12, 2019

Thanks @digulla !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants