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

tests: string and regexp literals #13

Merged
merged 1 commit into from
Nov 21, 2014

Conversation

mk-pmb
Copy link
Contributor

@mk-pmb mk-pmb commented Nov 17, 2014

Let's test some string and regexp literals that are not comments. Thus they shouldn't be stripped, and they aren't, so this test passes.

@tunnckoCore
Copy link
Collaborator

Lol. I added it to branch. I dont want to add passing test to master branch before we edit that bug.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Nov 17, 2014

I wonder how you can close a PR I sent to another user. (Update: I see, "collaborator" is the feature.)
Still, @jonschlinkert , I think it would be nice to have a common base commit with all tests passing, then all branches with failing literals can have that base commit as a parent.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Nov 17, 2014

I think @tunnckoCore meant this branch: https://github.com/jonschlinkert/strip-comments/compare/latest%2Bquotes_bug
In any case, please allow for my test code to have my authorship in its commit, which I can't see in that other branch. I'll supply a PR for the snake test[1] very soon after you accept the base commit.

[1] mk-pmb@ecaae07

@jonschlinkert
Copy link
Owner

@tunnckoCore let's please discuss prs before closing. I've been really busy and have a cold right now, so I'm not as active on github (besides what I need to get done for deadlines).

more importantly, we absolutely must always give property attribution for authorship and retain the original commit history whenever possible. I prefer merging pr's first. then updating if necessary, before publishing a new version to npm. This is just the cleanest, most transparent way to do it IMHO.

@jonschlinkert
Copy link
Owner

fwiw I really appreciate the contributions from you both. means a lot

@jonschlinkert
Copy link
Owner

lol I just noticed that my use stric typo is still there, and it has multiplied!: https://github.com/jonschlinkert/strip-comments/compare/latest%2Bquotes_bug#diff-321089e8814f6dca9ffed75810612a8dR1

@tunnckoCore
Copy link
Collaborator

@jonschlinkert: lol I just noticed that my use stric typo is still there, and it has multiplied!

Hahaha.. wtf, how I missed this, LOL! Thanks.

As for the PRs.. You're right, I'm also for transparency, no prob. It's my fault for this insane closing, sry.
I created the cli branch to hear your thoughts specially and for pinging you to review. It was easy to just direct commit it to master.

Branches, at all, is good thing, but not tons of branches - partially agree?

@mk-pmb: it would be nice to have a common base commit with all tests passing

We have, and for that I dont like the idea to add some partially passing test to master. Because it's not fully view of the use case. It's only some file with some not finished use-case test.

@tunnckoCore
Copy link
Collaborator

I think to add this test when we found cure for #12

@jonschlinkert
Copy link
Owner

not tons of branches - partially agree?

It's not good or bad, it depends on the reason for having the branches. However, regardless of whether we're talking about branches, or commits, or some other git concept, my view is that authorship and contribution history are more important than any other conventions or idioms. sometimes you squash, sometimes you ask an author to do another pr, etc. etc. whatever the situation I would always try to protect the author's contributions.

how I missed this, LOL!

I know lol, even after you gave me a hard time for it ;)

@tunnckoCore
Copy link
Collaborator

Yea, right. However, ball is on your side. :)

@jonschlinkert
Copy link
Owner

k, but pls give me extra time to get this done. we're trying to launch assemble/verb atm (we were supposed to last week but I got this damn cold and it slowed me down)

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Nov 17, 2014

I just noticed that my use stric typo is still there, and it has multiplied!

wtf, how I missed this, LOL! Thanks.

jslint wouldn't have tolerated this. jshint might have an option to ensure strict mode, too.

@jonschlinkert , please don't wait for #12 to be fixed. Having #13 as a base would help me demonstrate that #12 cannot be fixed with current architecture. This way, I hope to be able to save any future contributors the waste of thought capacity in attempt to improve the RegExp approach.

My plan is to provide counter-examples for any attempt to solve #12 by RegExp, starting with those from #1 back in April. Math predicts I can always do that (Chomsky hierarchy), which is exactly why it makes no sense to wait for a fix. :-)

@tunnckoCore
Copy link
Collaborator

Disagree. You can demonstrate it with #13 in one time, in PRs, forks and etc. without waiting for #12. Dont understand me uncorrect, im open for ideas to fix #12 but without changing architecture / adding esprima, and/or more complexity.

@jonschlinkert
Copy link
Owner

wait, so this pr is for adding tests only? I admit, I didn't look closely since I'm not feeling well. But I can't understand the logic behind not merging in tests. @tunnckoCore let's merge these tests in and then we can focus on discussing any architectural changes on their own merits.

jslint wouldn't have tolerated this

lol, yeah, you got me there. I sometimes don't run jshint on the test/ dir. I guess I should now that jshint has better support for mocha

@jonschlinkert
Copy link
Owner

ps fwiw, I've mentioned before on this project that we will need to use a non-regex solution to solve more complex use cases. I do don't know what the speed implications or other tradeoffs are, if any. but we can burn that bridge when we get there ;)

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Nov 18, 2014

Yes, #13 is just some simple test case. Since I can't convince @tunnckoCore by math alone:

im open for ideas to fix #12 but without changing architecture

... I'll instead try to convince him by examples. I spent quite some time yesterday discussing the formalities of how to submit them, so I hope this test will establish a framework for how to submit future tests.

Delaying #13 until someone discovers a way to cheat the Chomsky hierarchy seems equivalent to dismissing the PR. (It may be possible to solve #12's current problem, but as explained, I'm going to extend it a lot.)

ps fwiw, I've mentioned before on this project that we will need to use a non-regex solution to solve more complex use cases.

That's good to know. My aim with #13 is to provide proof for this insight.

@jonschlinkert jonschlinkert reopened this Nov 21, 2014
jonschlinkert added a commit that referenced this pull request Nov 21, 2014
@jonschlinkert jonschlinkert merged commit 6ff3690 into jonschlinkert:master Nov 21, 2014
@jonschlinkert
Copy link
Owner

@mk-pmb thanks, looking forward to seeing what you come up with.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Nov 21, 2014

Thanks! With this, #15 became a simple one-line change that leaves no doubts about why the test fails.

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

Successfully merging this pull request may close these issues.

3 participants