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

rootURL: replace backslashes with forward slashes, fixes #427 #428

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Nov 29, 2019

This might be not the right place to do the replacement, but it does the thing and unblocks my deploys.

@RobbieTheWagner
Copy link
Member

@lolmaus this seems to break the tests. Can you elaborate on why this is needed? Is it that windows generates paths with backslashes, then we need to convert them back? If would be great if we could get the existing stuff to always return the right paths, rather than manually changing them.

@lolmaus
Copy link
Contributor Author

lolmaus commented Nov 29, 2019

this seems to break the tests.

I don't think so. My branch is red because the master has been red when I branched off.

This seems to be the offending commit: https://travis-ci.org/ember-learn/ember-cli-addon-docs/builds/607226331 At least its commit message sounds related to the assertion error message.


Can you elaborate on why this is needed? Is it that windows generates paths with backslashes, then we need to convert them back?

Yes.


If would be great if we could get the existing stuff to always return the right paths, rather than manually changing them.

Well, yes, I generally agree, as I said in the top message. But I don't know where exactly the issue happens, and debugging Node is tedious.

My solution seems to be adequate and robust.

@RobbieTheWagner
Copy link
Member

@lolmaus can you rebase this please so we can see if the tests pass?

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 9, 2019

@rwwagner90 Passing now!

@RobbieTheWagner RobbieTheWagner merged commit b3df7dd into ember-learn:master Dec 9, 2019
@RobbieTheWagner
Copy link
Member

Thanks for the PR!

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

Successfully merging this pull request may close these issues.

2 participants