Improve regex for finding existing routes #1371
Conversation
|
Can you add a failing test? |
| @@ -20,7 +20,7 @@ function addRouteToRouter(name, options) { | |||
| var type = options.type || 'route'; | |||
| var routerPath = path.join(process.cwd(), 'app', 'router.js'); | |||
| var oldContent = fs.readFileSync(routerPath, 'utf-8'); | |||
| var existence = new RegExp("(route|resource)\\(['\"]" + name + "'"); | |||
| var existence = new RegExp("(?:route|resource)\\s?\\(\\s?(['\"])" + name + "\\1"); | |||
There was a problem hiding this comment.
I think, this is complex enough to warrant an explanation:
(?:route|resource) // non-capturing match for literal "route" or "resource"
\s? // zero or one white space
\( // opening literal parantheses (
\s? // zero or one white space
(['\"]) // matching/capturing for single or double quote
name-variable
\1 // matching the previously captured single or double quote
One tiny suggestion: wouldn't \s* make more sense than \s? ? In both cases, it would be syntactically correct to write route . . . . ( . . . "post" ) (dots intentional for more spacing)
There was a problem hiding this comment.
I agree on the change.
Are you suggesting I add this explanation inline, or just for clarification here?
There was a problem hiding this comment.
On github, it is very easy to miss. Especially if the code is changed later-on. It surely doesn't need to be as big a block of text as my attempt (i was merely writing along as i was reading the regex). Sadly, JavaScript does not have the x mode that ruby has. This would be super cool.
|
I'm fine for this right now but going forward we should likely just use esprima for this, although escodgen doesn't support es6 syntax yet, we can simply replace the subgraph of code specific to the routes and splice it back in. |
|
Added some tests. @stefanpenner Yeah, I can see that being a good option. |
Improve regex for finding existing routes
Some improvements to the check for existing routes:
"now works in addition to'. (This looks like it was an oversight)