fix(#964): Allow "." in variable names + make error message more consistent #971
Conversation
|
Hi @helloanoop 👋, when I opened this PR I tested everything manually and everything seemed alright, but I had some questions about the e2e & unit tests:
|
| @@ -1 +1 @@ | |||
| export const envVariableNameRegex = /^(?!\d)[\w-]*$/; | |||
| export const envVariableNameRegex = /^(?!\d)[\w-.]*$/; | |||
There was a problem hiding this comment.
I believe . needs to be escaped in order to be considered a literal ., otherwise, . refers to any character.
There was a problem hiding this comment.
Within a character set/class ([]) the . is treated as a literal instead of the usual "match any character" 👍
(side note: I used both https://regexr.com/ & https://regex101.com/ while testing, and the former showed no issues with this regex, but the latter displayed an error for the - which can be solved by escaping it. Not sure why there's a difference between them since they should both use the browser's JS regex engine)
| @@ -1,7 +1,7 @@ | |||
| const Handlebars = require('handlebars'); | |||
| const { cloneDeep } = require('lodash'); | |||
|
|
|||
| const envVariableNameRegex = /^(?!\d)[\w-]*$/; | |||
| const envVariableNameRegex = /^(?!\d)[\w-.]*$/; | |||
There was a problem hiding this comment.
Why don't we import this from packages/bruno-app/src/utils/common/regex.js?
There was a problem hiding this comment.
Great point, I agree and I'd also prefer to import it from one place instead of duplicating it. I wasn't sure whether to refactor it in this PR or a separate PR (not sure how it would be preferred in this project).
In this specific case, the bruno-app package is the standalone Next.js app, whereas the bruno-js package is a library used by the other packages in the codebase, so I think we can export the regex from bruno-js (or maybe another common/util package).
There was a problem hiding this comment.
I'm thinking of refactoring after all, otherwise I'll create a separate PR to keep this one small/focused
@helloanoop what do you think?
…n in string only to variables that are not process env vars
…R + add test case for escaped vars
Closes #964
Thanks for creating this project btw! I was getting really disappointed with the alternatives (Insomnia, Postman, etc.) before stumbling upon Bruno on HN, and it's exactly what I was looking for.
Description
Added
.to the Regex, removed the now unnecessary check with the old regex & changed the error messages to be a bit more consistent.Also added literal-segment notation with
[]around the var name when interpolating since Handlebars doesn't allow.Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.