Skip to content

Conversation

@PerthCharern
Copy link
Contributor

@PerthCharern PerthCharern commented Sep 26, 2018

Fix #287

  • Allow composite expression with the following formats:

    • Strings with runtime expression embedded in { }
    • Plain strings without any runtime expression
  • Strings with {$xxxx} where xxxx doesn't constitute a valid runtime expression is NOT allowed. A set of braces with a dollar sign denotes a runtime expression in a composite expression and should not be used in other random scenarios.

    • Note that the spec is a bit ambiguous in this regard. From a literal reading of the spec, the requirement is way more generous (any string should be okay). If we decide to allow that, then this requirement can be lifted later without being a breaking change.
  • Runtime expressions should not be static. Theoretically, we don't want any classes in our object to be static. If the UrlExpression class is modified in one object in any arbitrary way, it should not effect a UrlExpression class in a different object.

  • Adjust unit tests

- Allow composite expression with the following formats:
  - Strings with runtime expression embedded in { }
  - Plain strings without any runtime expression
- Strings with {$xxxx} where xxxx doesn't constitute a valid runtime expression is NOT allowed. A set of braces with a dolalr sign denotes a runtime expression in a composite expression and should not be used in other random scenarios.

- Adjust unit tests
@PerthCharern
Copy link
Contributor Author

PerthCharern commented Sep 26, 2018

@Shwetap05 @darrelmiller Do we have a consensus based on #313 on which branch to use? Are we still using vNext? Based on my understanding, we are doing this:

  • Bug fix: Both master and vNext
  • Feature release: Only vNext

The change in this PR should then be merged into both master and vNext.

@Shwetap05
Copy link
Member

@PerthCharern we discussed to stick with vNext for preview features and yes bug fixes should go to both master and vNext

Copy link
Member

@Shwetap05 Shwetap05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@PerthCharern PerthCharern merged commit d71e9b0 into master Sep 26, 2018
@PerthCharern PerthCharern deleted the perthcharern/AllowCompositeExpression branch September 26, 2018 21:19
@PerthCharern PerthCharern restored the perthcharern/AllowCompositeExpression branch September 26, 2018 21:19
@PerthCharern PerthCharern deleted the perthcharern/AllowCompositeExpression branch September 26, 2018 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants