Skip to content

Conversation

@anubh-v
Copy link
Contributor

@anubh-v anubh-v commented Apr 13, 2020

Currently, when we create the Share URL for the Non-deterministic and Concurrent variants, we encode the chapter and variant information together in one string.

For example, if the playground was using Source 3 Non-Det, then then Share URL will look
like this https://source-academy.github.io/playground#chap=3_Non_Det&exec=1000&ext=NONE&prgrm=IYWwRgFAjAlA3EA

Note the string #chap=3_Non_Det in the URL.

Instead of this, we can encode the chapter and variant separately.
So, the url will look like: https://source-academy.github.io/playground#chap=3&exec=1000&ext=NONE&prgrm=IYWwRgFAjAlA3EA&variant=non-det

This PR reduces the steps needed to add a new language variant.
The Share URL will automatically support any language variants added into the sourceLangauges array in src/reducers/states.ts.

Note: The variant field in the Share URL is optional
If the URL does not have a variant, the playground will use the default variant.
This ensures the Share URL is backward compatible

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4445

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 37.154%

Totals Coverage Status
Change from base Build 4443: -0.01%
Covered Lines: 2800
Relevant Lines: 6748

💛 - Coveralls

@anubh-v
Copy link
Contributor Author

anubh-v commented Apr 13, 2020

If the URL has a chapter - variant combination that is not supported, then the variant will be set to default.

One example is if the URL has chapter = 4, but variant = non-det

@anubh-v anubh-v changed the title Encode chapter and variant separately in share URL Encode chapter and variant separately in Share URL Apr 13, 2020
@martin-henz
Copy link
Member

If the URL has a chapter - variant combination that is not supported, then the variant will be set to default.

One example is if the URL has chapter = 4, but variant = non-det

Not sure if that's the right choice. I see four options:
(1) change variant to "default"
(2) increase chapter until a matching variant exists
(3) decrease chapter until a matching variant exists
(4) reject program

I think in any case (except perhaps 2) we should give a warning.

A clear and simple solution would be simply (4) with an error message.

If you want to be fancy, you could:

  • Follow (2), if not possible:
  • Follow (3) and give warning, if not possible:
  • Follow (1) with a warning, if not possible:
  • Follow (4) with an error message.

@martin-henz martin-henz self-requested a review April 13, 2020 09:33
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

Regarding the question what to do if we dont have a chapter with this variant:

If the URL has a chapter - variant combination that is not supported, then the variant will be set to default.

One example is if the URL has chapter = 4, but variant = non-det

Not sure if that's the right choice. I see four options:
(1) change variant to "default"
(2) increase chapter until a matching variant exists
(3) decrease chapter until a matching variant exists
(4) reject program

I think in any case (except perhaps 2) we should give a warning.

A clear and simple solution would be simply (4) with an error message.

If you want to be fancy, you could:

  • Follow (2), if not possible:
  • Follow (3) and give warning, if not possible:
  • Follow (1) with a warning, if not possible:
  • Follow (4) with an error message.

@martin-henz martin-henz merged commit ad05a01 into source-academy:sa_2021 Apr 13, 2020
btzy pushed a commit that referenced this pull request May 2, 2020
* Encode chapter and variant separately in URL

* Use variant in url test
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