Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Windows fixes #199

Merged
merged 5 commits into from May 26, 2016
Merged

Windows fixes #199

merged 5 commits into from May 26, 2016

Conversation

justinfagnani
Copy link
Contributor

No description provided.

@justinfagnani
Copy link
Contributor Author

Not ready for review yet. 3 tests still failing.

@tenosiswono
Copy link

You should fix on github.ts too. It has path problem in windows, all the files ignored for being extracted.

@justinfagnani
Copy link
Contributor Author

I know. First step is to get ask existing tests passing.

@justinfagnani
Copy link
Contributor Author

@FredKSchott ready for review now!

if (file) {
deferred.resolve(file.contents.toString());
} else {
throw new Error(`No file found for ${filepath}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferred.reject?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that and it ended up being wrong because Hydrolysis actually rejects... I left the line in commented out with a comment not to add it back :)

@FredKSchott
Copy link
Contributor

Looking good, a few small comments

@FredKSchott
Copy link
Contributor

@tennosys would love your feedback as well if you have a chance to check out this branch and confirm that it fixes any bugs you were seeing.

let url = this.urlFromPath(fragment)
let file = this.files.get(url);
if (file == null) {
throw new Error(`no file found for fragment ${fragment}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error or done(error)?

I know, These are annoying :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@FredKSchott
Copy link
Contributor

LGTM!

Lets definitely test this in all of our major workflows / give it some time to bake before releasing to npm.

@justinfagnani justinfagnani force-pushed the windows branch 2 times, most recently from b82cf4c to ce25cf0 Compare May 26, 2016 01:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants