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

[$500] Prevent nesting code blocks in markdown #27208

Closed
thienlnam opened this issue Sep 12, 2023 · 44 comments
Closed

[$500] Prevent nesting code blocks in markdown #27208

thienlnam opened this issue Sep 12, 2023 · 44 comments
Assignees
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@thienlnam
Copy link
Contributor

thienlnam commented Sep 12, 2023

We've been running into all sorts of issues by allowing nested markdown rules.

Ex: #23034
Ex: #26941

Context: https://expensify.slack.com/archives/C01GTK53T8Q/p1694011229493309

As one of the new rules, let's prevent nested markdown

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f8d855ec7167b7ca
  • Upwork Job ID: 1701416634058772480
  • Last Price Increase: 2023-10-10
  • Automatic offers:
    • alitoshmatov | Contributor | 27885116
@thienlnam thienlnam added the Daily KSv2 label Sep 12, 2023
@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Sep 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01f8d855ec7167b7ca

@melvin-bot melvin-bot bot changed the title Prevent nested markdown [$500] Prevent nested markdown Sep 12, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

@alitoshmatov
Copy link
Contributor

Just to clarify is Hello world also nested markdown? Or is it only for cases with code-fence?

@thienlnam
Copy link
Contributor Author

Good question, I think code fence situations only

@thienlnam thienlnam changed the title [$500] Prevent nested markdown [$500] Prevent nested markdown for code blocks Sep 12, 2023
@thienlnam thienlnam changed the title [$500] Prevent nested markdown for code blocks [$500] Prevent nested markdown in code blocks Sep 12, 2023
@alitoshmatov
Copy link
Contributor

alitoshmatov commented Sep 12, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Prevent nested markdown in code blocks

What is the root cause of that problem?

I tested all combinations with nested code blocks and it looks like only strikethrough is applied to nested code blocks.
For example here are results of doing *```bold```*, _```italic```_, ~```strike```~
Screenshot 2023-09-12 at 10 36 40

This is because in all other cases except strike-through we are checking if nested content contains <pre>, which means codeblock, we do not apply the styling.

https://github.com/Expensify/expensify-common/blob/35bff866a8d345b460ea6256f0a0f0a8a7f81086/lib/ExpensiMark.js#L209

Only in strikethrough we do not have this condition

https://github.com/Expensify/expensify-common/blob/35bff866a8d345b460ea6256f0a0f0a8a7f81086/lib/ExpensiMark.js#L214

What changes do you think we should make in order to solve the problem?

I think adding g1.includes('<pre>') condition should resolve the issue:

  replacement: (match, g1) => (g1.includes('<pre>') ||this.containsNonPairTag(g1) ? match : `<del>${g1}</del>`)

And the result will be consistent with others:
Screenshot 2023-09-12 at 10 44 14

What alternative solutions did you explore? (Optional)

@cubuspl42
Copy link
Contributor

@thienlnam This issue is mistitled, you are talking about fenced code blocks nested in markdown (inlines), not the other way around.

@parasharrajat
Copy link
Member

Yeah, it should be Prevent nesting code blocks in markdown.

@parasharrajat
Copy link
Member

Another example of such nested rules #26941

@cubuspl42
Copy link
Contributor

@parasharrajat Do you suggest extending the scope of this issue and closing the other one?

@thienlnam thienlnam changed the title [$500] Prevent nested markdown in code blocks [$500] Prevent nesting code blocks in markdown Sep 13, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@parasharrajat, @stephanieelliott Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@parasharrajat
Copy link
Member

I will pick this up tomorrow as a new project as it seems there is more to it than just being an issue.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

@parasharrajat, @stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Sep 26, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

@parasharrajat, @stephanieelliott 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

@parasharrajat, @stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

@parasharrajat, @stephanieelliott 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@parasharrajat
Copy link
Member

Wasn't able to test this yet.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 30, 2023

@parasharrajat, @stephanieelliott Eep! 4 days overdue now. Issues have feelings too...

@stephanieelliott
Copy link
Contributor

Hey @parasharrajat any update on this? Seeing as this we're treating this as more of a project and it doesn't have any impact on functionality, I'm going to move this to Weekly

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@stephanieelliott stephanieelliott added Weekly KSv2 Overdue and removed Daily KSv2 labels Oct 30, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@stephanieelliott
Copy link
Contributor

Bump @parasharrajat, any update?

@stephanieelliott
Copy link
Contributor

Hey @parasharrajat it's been a while -- can you give an update please?

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2023
@stephanieelliott
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Nov 29, 2023
@parasharrajat
Copy link
Member

parasharrajat commented Nov 29, 2023

@alitoshmatov's proposal makes sense. Let's go with it.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Nov 29, 2023

Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Copy link

melvin-bot bot commented Nov 29, 2023

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($500)

Copy link

melvin-bot bot commented Nov 29, 2023

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@alitoshmatov
Copy link
Contributor

Looks like this issue was resolved in #24281 which applied the same solution as I proposed.

@parasharrajat
Copy link
Member

Oh... Sorry for the delay in picking up your proposal early. I closed 3 issues in favor of this one but someone must have reported a duplicate issue again. I will take note of this.

So for now, there is nothing to do here. I guess.
cc: @stephanieelliott

@s77rt
Copy link
Contributor

s77rt commented Dec 11, 2023

This was closed without fixing the other linked issue #26941

@parasharrajat
Copy link
Member

parasharrajat commented Dec 11, 2023

@s77rt you can continue on that issue to find a solution. There is nothing done on this issue, it was solved indirectly from other duplicate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants