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

fix(#1143): Fix PR #971 - Use literal segments only for env/collection variables + Add to CLI #1154

Conversation

nelup20
Copy link
Contributor

@nelup20 nelup20 commented Dec 5, 2023

Closes #1143, Closes #1148, Closes #1150, Closes #1367 & Closes #1321
Fixes changes introduced by PR #971

Description

Now we add [] only to variables within a string & only to variables which are not process environment variables (those that don't start with process.env.).

I also added this feature to the CLI.

Before After
bruno_bugfix_1143_cli_before.mp4
bruno_bugfix_1143_cli_after.mp4
bruno_bugfix_1143_electron_app_before-1.mp4
bruno_bugfix_1143_electron_app_after-1.mp4
bruno_bugfix_1143_electron_app_before-2.mp4
bruno_bugfix_1143_electron_app_after-2.mp4
bruno_bugfix_1143_electron_app_before-3.mp4
bruno_bugfix_1143_electron_app_after-3.mp4

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

@Its-treason
Copy link
Member

Its-treason commented Jan 18, 2024

So this is a good start, but I don't really like the regex.

image

Your regex did affect variables that either did include a dot or were escaped using a \.

So i extended your Regex to: /(?<!\\)\{\{(?!process\.env\.\w+)(.*\..*)\}\}/g on regexr: https://regexr.com/7ql6d

image

Small regex dissection:

  • (?<!\\) Negative look behind to ignore escaped variables
  • (?!process\.env\.\w+) negative look ahead to unmatch process.env.*
  • (.*\..*) Match anything that includes a dot

After I saw Martin's reply here: #1410 (reply in thread) I thought maybe we can use the missingHelper here, but apparently the missingHelper does not work with nested properties: handlebars-lang/handlebars.js#1435.

@nelup20
Copy link
Contributor Author

nelup20 commented Jan 20, 2024

Thanks for improving the regex @Its-treason, I've changed it to yours + added a test case for escaped vars.

Unfortunately, I won't have time anymore to contribute to Bruno (and subsequently follow up on this PR in case of issues), so I won't be able to look into a better solution using Handlebars' features/options. I've allowed edits for maintainers so feel free to take over this PR or close it as you see fit 👍

@helloanoop helloanoop merged commit 7de5bbb into usebruno:main Jan 25, 2024
2 checks passed
@helloanoop
Copy link
Contributor

Thank you so much @nelup20 ! 💛

My apologies. This should have been merged long back.

Thank you so much @Its-treason for the review! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment