-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add tests and more #47
Conversation
This is some great work! I've added a few comments for things to consider. Additionally, the build is failing because:
I haven't dug into why it cannot find stats yet. |
I absolutely don't understand this error.... As my dev friend often says 'it works on my machine'. |
Definitely no idea about compilation issue. @carlowahlstedt can you please have a look ? |
Finally it's OK ;-) |
Not ignoring you here, just been really busy. I'll try to get back to looking at this over the weekend. I appreciate your work and patience on this. |
So...hadn't really used the PR changes before. I had started comments days ago, but didn't realize I had to submit them before you could see them. Either way, they are there now. Overall everything looks really good and I think your tests are probably a big testament to that. I'm thinking we will release this to a new major version (so people have to manually upgrade) since there are several potential breaking changes here. |
Ok with your comments. |
This PR aims to :
Please do not consider PR #45, since this one handles it.
I assume all unit test are Ok, but no clues if it will work with newman (especially URL's support) but according to #46 (comment), it should work like a charm...
I added radio to specify if one use file url or file, default is file for compatibility