-
Notifications
You must be signed in to change notification settings - Fork 4
Fix crash + add configuration support #2
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
base: master
Are you sure you want to change the base?
Conversation
These look like very good improvements, thank you for this! However, I'd really prefer if you'd split the unit tests changes into a separate PR, so I can make sure that the original test cases still pass. I don't really use this library anymore (so I don't have good test cases handy), but my former employer still might, and I'd hate to break something in their build pipeline. |
Wouldn't it be easier to just copy the new
The old test cases/fixtures are unchanged. Only the runner is different: |
31d0538
to
a9c5bec
Compare
I'd rather not make a PR with new features that are untested, particularly with old libraries that are now deprecated:
I think the safest way to manage this, if you're worried about it impacting a legacy system you no longer have access to, is as a new major release, i.e. v2.0.0 — although, as indicated above, this is a non-breaking change. If you want to justify the bump, it can be on the basis that, while it still works, Babel 6 is no longer supported... Incidentally, I'm happy to take over maintenance of this module, if you're no longer have the time or inclination. |
- fix crash ("Maximum call stack size exceeded") when a substitution includes a parameter name - support selecting the function(s) to inline via an annotation (comment) or label to avoid the need to mangle function names - support custom prefixes, e.g. "__inline__" rather than "__INLINE__" - rejig the tests to use snapshots rather than manually creating every fixture - documentation overhaul - add missing description + repo link to the package.json - add missing license
a9c5bec
to
c8c1b74
Compare
I get your point. Can you do it the other way around, please? My point is that re-working the tests and the code at the same time no longer guarantees the same behaviour. Especially changing test runners and such, unprompted. When adding new features, I'd like to see only additions in the test code, not a complete rewrite. |
Sorry, not clear what you're requesting?
Is there something here (or here) that suggests the behavior has changed?
As mentioned above, the old tests are completely unchanged. Only the runner has changed. |
Seeing this makes me worried. It wouldn't be a big deal if I could test this properly myself, but I don't have access to the environment I previously used this module anymore; and I'd just like to be sure the current behaviour is unchanged. What I'm requesting is to split out your code changes from your test runner changes (still not clear about why you'd change that). If you want to improve the tests, be my guest, but don't change the code that goes with it. If you want to improve the functionality, again, you're welcome to do so, but I don't want to see any "red" in the I really want these changes to be separate. Thank you. |
I thought that was clear from the description?
There are 11 tests now whereas before there were only 4, so not wasting time creating fixtures that can be created automatically was the motivation. Also, the test runner code (old -> new) is smaller and simpler. I can't see any actual objections to it beyond the fact that it's shorter (i.e. "red" lines in the diff). Also, Mocha 2.x is out-of-date and has several serious vulnerabilities.
|
Okay, thanks for clarifying the rationale for the test changes, it does seem like an improvement. However, I'd still like to see them as a separate PR. |
"__inline__"
rather than"__INLINE__"
package.json
Closes #1