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

Fixed tests #18

Merged
merged 15 commits into from Apr 9, 2019

Conversation

Projects
None yet
2 participants
@JulianWielga
Copy link
Contributor

commented Mar 27, 2019

fixed test to allow checking of PR's with new modules.

removed checking (and downloading) all versions in range, which was in my opinion really bad idea. instead only edge versions of each range are chosen.

bumped dependencies versions to latest => dropped node 4.

JulianWielga and others added some commits Mar 27, 2019

@mastilver

This comment has been minimized.

Copy link
Owner

commented Apr 5, 2019

removed checking (and downloading) all versions in range, which was in my opinion really bad idea. instead only edge versions of each range are chosen.

Yep, that quite clever I didn't think about that 👍

@mastilver
Copy link
Owner

left a comment

lgtm, few a few minor things

I think your compromise is good, we could potentially lose on accuracy by not testing every single versions but at the state it's currently I get spammed by travis-ci so it's the best solution

Show resolved Hide resolved .travis.yml
"development": "https://unpkg.com/rxjs@[version]/bundles/Rx.js",
"production": "https://unpkg.com/rxjs@[version]/bundles/Rx.min.js"
}
},
"$disabled": {

This comment has been minimized.

Copy link
@mastilver

mastilver Apr 5, 2019

Owner

what is that?

This comment has been minimized.

Copy link
@JulianWielga

JulianWielga Apr 9, 2019

Author Contributor

json has no comments. rxjs 6+ has different var name. it's not supported by your modules. I wanted to make some PR later, after ci start working without fail. also I wanted to make sure that everyone can see that it makes no sense to add 6+.

This comment has been minimized.

Copy link
@mastilver

mastilver Apr 9, 2019

Owner

ok, I see do you mind replacing it with a comment and use https://github.com/sindresorhus/strip-json-comments

- const modules = require('./modules');
+ const modules = stripJsonComments(fs.readFileSync(path.join(__dirname, './modules')))

This comment has been minimized.

Copy link
@mastilver

mastilver Apr 9, 2019

Owner

it's not a blocker, I could take care of that, I will merge your PR by tomorrow regardless

This comment has been minimized.

Copy link
@JulianWielga

JulianWielga Apr 9, 2019

Author Contributor

I don't like the idea of "custom" comments and stripping them with some tools. this will break editors, formatters etc. json is json and we should live with that. maybe later you should switch to yaml or sth. i'll remove this "commented" part, you're right, there should be no commented code in repo.

JulianWielga added some commits Apr 9, 2019

@mastilver mastilver merged commit f9572ac into mastilver:master Apr 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mastilver

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2019

@JulianWielga Thank you!

phillip-kil added a commit to phillip-kil/module-to-cdn that referenced this pull request Apr 11, 2019

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.