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] Highlight unterminated quoted strings differently than correctly terminated quoted strings. #4165

Merged
merged 7 commits into from
Mar 23, 2023

Conversation

antoineveldhoven
Copy link
Collaborator

@antoineveldhoven antoineveldhoven commented Mar 22, 2023

Description

  • Remove styling for unterminated quoted strings in Level 4;
    For educational purposes (the essence of Level 4) do not highlight incorrect termination of quotations.
  • Mark unterminated quoted strings as invalid for Level 5 and above

Fixes #3768

How to test

Level 4 and above

Checklist
Done? Check if you have it all in place using this list:*

  • Contains one of the PR categories in the name
  • Describes changes in the format above
  • Links to an existing issue or discussion
  • Has a "How to test" section

@Felienne
Copy link
Member

Thanks @antoineveldhoven!! This highlighting for level 4 for sure is better than what we have now!

But I am not sure I like the "heavy" highlighting in the higher levels. It is not bad in itself, but it is a bit out of tone maybe with what we have for other errors.

Shall we do it in steps and first highlight unterminated strings in white in all levels? Then we can experiment with the pink later on separately (I would love to test drive it in class for a bit)

@Felienne
Copy link
Member

Ow yes and the highlighting has tests that are now broken, do let us know if you need help to fix them!

@antoineveldhoven
Copy link
Collaborator Author

Ow yes and the highlighting has tests that are now broken, do let us know if you need help to fix them!

First time commiter here 🙋‍♂️ 🤦‍♂️
Will have a (better) look at the tests and resubmit my PR.

@Felienne
Copy link
Member

Felienne commented Mar 22, 2023

Ow yes and the highlighting has tests that are now broken, do let us know if you need help to fix them!

First time commiter here 🙋‍♂️ 🤦‍♂️ Will have a (better) look at the tests and resubmit my PR.

No problemo! That was actually why the tests did not run (first timers have to be manually approved). Do reach out if we can help (here or in Discord)

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
@antoineveldhoven
Copy link
Collaborator Author

Ow yes and the highlighting has tests that are now broken, do let us know if you need help to fix them!

First time commiter here 🙋‍♂️ 🤦‍♂️ Will have a (better) look at the tests and resubmit my PR.

No problemo! That was actually why the tests did not run (first timers have to be manually approved). Do reach out if we can help (here or in Discord)

Thnx for the review; I've updated this PR, can you have another look, assuming the tests run fine this time 😜

@Felienne
Copy link
Member

Ow yes and the highlighting has tests that are now broken, do let us know if you need help to fix them!

First time commiter here 🙋‍♂️ 🤦‍♂️ Will have a (better) look at the tests and resubmit my PR.

No problemo! That was actually why the tests did not run (first timers have to be manually approved). Do reach out if we can help (here or in Discord)

Thnx for the review; I've updated this PR, can you have another look, assuming the tests run fine this time 😜

Let's see ;) I will also send you an invite to the repo, that way your PRs will run the tests automatically!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Thanks @antoineveldhoven, all good now!

And congrats on getting your first PR approved!!! 🥳

@mergify
Copy link
Contributor

mergify bot commented Mar 23, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7e684cf into hedyorg:main Mar 23, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 23, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@antoineveldhoven
Copy link
Collaborator Author

Thanks @antoineveldhoven, all good now!

And congrats on getting your first PR approved!!! 🥳

thnx, also for the fix in the print test!

@antoineveldhoven antoineveldhoven deleted the issues/3768 branch March 23, 2023 09:16
@Felienne
Copy link
Member

I have also opened a Discussion for your other idea: #4169 Will ask a few teachers to chip in with their opinion!

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.

[HIGHLIGHTING] Do not highlight strings that do not end in a quote
2 participants