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

demo broken #21

Closed
DanielRuf opened this issue Dec 16, 2013 · 8 comments
Closed

demo broken #21

DanielRuf opened this issue Dec 16, 2013 · 8 comments

Comments

@DanielRuf
Copy link

It seems there are some issues with the demo?
I see only 2 errors in the console in Google Chrome and it does not work.

@benjojo
Copy link
Contributor

benjojo commented Dec 16, 2013

Looks like this is the cause:

rg8wxxr

Even stranger it seems to have come from here: https://github.com/younesherlock/Knwl.js/blob/cfebb166e2fdda7e5f15bc7a74935fcc9be637a4/knwl.js (Not pushed into the main yet but it is from pull request #20)

@DanielRuf
Copy link
Author

This is very creepy. Regex is cool but using regex to replace all the readable source code is just insane.
I would suggest a revert.

http://www.codinghorror.com/blog/2008/06/regular-expressions-now-you-have-two-problems.html

overuse of regular expressions is evil.
...
If you read an incredibly complex, impossible to decipher regular expression in your codebase, they did it wrong. If you write regular expressions that are difficult to read into your codebase, you are doing it wrong.
...
Do not try to do everything in one uber-regex. I know you can do it that way, but you're not going to. It's not worth it. Break the operation down into several smaller, more understandable regular expressions, and apply each in turn. Nobody will be able to understand or debug that monster 20-line regex, but they might just have a fighting chance at understanding and debugging five mini regexes.

Use whitespace and comments. It isn't 1997 any more. A tiny ultra-condensed regex is no longer a virtue. Flip on the IgnorePatternWhitespace option, then use that whitespace to make your regex easier for us human beings to parse and understand. Comment liberally.

http://programmers.stackexchange.com/questions/113237/when-you-should-not-use-regular-expressions

@benjojo
Copy link
Contributor

benjojo commented Dec 16, 2013

The thing is, That commit I ref'd is not even merged yet. It should not be on the site.

@benjojo
Copy link
Contributor

benjojo commented Dec 16, 2013

Yes but that commit that is on the demo site is not in the repo,
its in @younesherlock's fork that is awaiting merge.
https://github.com/younesherlock/Knwl.js/commit/cfebb166e2fdda7e5f15bc7a74935fcc9be637a4

@DanielRuf
Copy link
Author

Oh ok.
I see you posted the wrong pull request link (19) and changed it to the right one (20) so this confused me.
Now it is clear. The sourcecode on the demo website is newer and includes unmerged commits and pull requests.

@younes-io
Copy link
Contributor

Sorry, I forgot to remove parentheses from the regex; it's from the RFC 3986 : http://tools.ietf.org/html/rfc3986#page-50

The regex should be like : ^[^:/?#]+:?//[^/?#]*?[^?#]*\?[^#]*?#.*?$

However, I don't know why it caused the problem in the demo since it's hasn't been merged.

@benhmoore
Copy link
Owner

Fixed demo, hopefully should work now!

@DanielRuf
Copy link
Author

👍 works now, this issue can be closed

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

No branches or pull requests

4 participants