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

BREAKING-CHANGE Preserver whitespace in variable fallback #5571

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@exoego
Copy link
Contributor

exoego commented Dec 6, 2018

What did you implement:

Closes #5558

How did you implement it:

Remove whitespace cleaning (.replace(/\s/g, '');) which seems unnecessary.

I am suprised that removing it break nothing.
I am not sure that it is essentially unnecessary...

How can we verify it:

npm test

Todos:

  • Write tests
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@@ -268,7 +268,7 @@ class Variables {
return match.replace(
this.variableSyntax,
(context, contents) => contents.trim()
).replace(/\s/g, '');
);

This comment has been minimized.

@dschep

dschep Dec 6, 2018

Member

I had to dive deep in the git blame rabbit hole.. but it seems you initially added the whitespace removal @eahefnawy. What are you thoughts on this PR?

This comment has been minimized.

@eahefnawy

eahefnawy Dec 6, 2018

Member

If I remember correctly, this is meant to normalize variables written like this ${opt: foo}. I think the real solution would be to NOT remove white spaces from the literal fallback only, but keep it for variables. This might be easier said than done though...

Overall, imo the issue raised is more severe than cleaning the variable itself. So I wouldn't oppose losing that tiny functionality.

@dschep dschep requested a review from eahefnawy Dec 6, 2018

@dschep dschep changed the title Preserver whitespace in variable fallback BREAKING-CHANGE Preserver whitespace in variable fallback Dec 6, 2018

@dschep

dschep approved these changes Dec 6, 2018

Copy link
Member

dschep left a comment

Thanks for the feedback @eahefnawy!

Adding a note to the subject line about this technically being a breaking change so I remember to call it out in the release notes

@dschep dschep merged commit 80f9a25 into serverless:master Dec 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@exoego exoego deleted the exoego:preserver-space-in-fallback branch Dec 7, 2018

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