-
Notifications
You must be signed in to change notification settings - Fork 30
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
[Request] Organize project and tests in multiple files for V2 #12
Comments
Hi! Thanks for the detail.
So I'm OK to do this.
What do you think? |
Yeah, test speed right now is pretty good. Hopefully it never applies here, but at work I've dealt with some older projects that have test suites that take up to 10-15 seconds to run. In response to your points:
|
I switched the project to CommonJS.
See: 4d7db7d...58a42c6 |
I split the code in 2 files and refactored tests. |
Continuing on the discussion from my PR, I think it would be valuable to split version 2 up into various smaller files that can be minified together for usage on the web, or used directly in Node.
There are a handful of benefits to this --
1. Easier for newcomers to grok
Understanding and contributing to foreign codebases can be a daunting task. One way to make that simpler is to divide the code into obvious chunks that can be consumed one at a time. While whitespace chunking and comments are generally sufficient for people who are familiar with the codebase, they require at least one (and possibly several) full read throughs of the codebase before a new programmer can reliably use them.
To give an example of this, I recently contributed to V1 to add support for the LIKE and IN operators. The actual changes were very simple (one line in
CondLexer
'sreadWord()
method), but actually finding that out was a fairly intensive process. If there had been a separate file for the CondLexer and CondParser, I would have quickly taken a look at one or the other, and if I had gone to CondParser, it would have lead me to CondLexer very quickly.Since you already have a build system in place via grunt, the actual cost of adding the minification step for the web is basically nil, you even get the benefit of more useful stack traces in node (assuming you use the non-catted files).
2. Easier to write comprehensive tests for
Like the main library, the test suite is currently also in a single file. If it were split into multiple files (preferably multiple files of many separate tests) along the same boundaries as the source files they test, I could have just that file of tests run on every change I make, alerting me immediately to mistakes rather than waiting until I break out of flow to manually run the tests and check. Running the entire test suite instead of just the relevant ones is much more time consuming and tends to only add noise to the signal.
3. Better editor/tooling support
Smaller files let tools such as JSHint/ESLint and JS-Beautifier run faster. Many editors (such as my own) offer integration with these tools that help ensure high quality code. However, on very large files, they can cause intrusive lag and need to be disabled.
The text was updated successfully, but these errors were encountered: