Skip to content

Add full equivalence between different types of linebreaks - #149

Open
tacc-tacc wants to merge 2 commits into
5j9:mainfrom
tacc-tacc:main
Open

Add full equivalence between different types of linebreaks#149
tacc-tacc wants to merge 2 commits into
5j9:mainfrom
tacc-tacc:main

Conversation

@tacc-tacc

Copy link
Copy Markdown
Contributor

Goal: to consider the three linebreaks (\n, \r, and \r\n) all the same while reading/paarsing wikitext.
Changes made:

  • On regex patterns, changed all occurences of \n and \r\n for \R, and [^\n...] replaced with [^\r\n...]
  • On regex patterns, replace all ^ and $ anchor/boundary tokens with (?<=\R|\A) and (?=\R\Z), respectively, when talinkg about multiline modes. On non multiline modes, just replace them with \A and \Z for better readability.
  • Remove MULTILINE FLAG since we don't need it anymore
  • On _table.py, added a new regex pattern to search for the first occurence of a linebreak (\R). I suggest to review further this file since I changed some lines of the logic/flow to adress multi-valued and variable-lengthed linebreaks, but I think it didn't produced any breakage in the functionality.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (5cbf028) to head (bf755e1).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #149   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         2264      2274   +10     
=========================================
+ Hits          2264      2274   +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tacc-tacc

Copy link
Copy Markdown
Contributor Author

@5j9

@5j9

5j9 commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Looks good overall. One thing I'd like to see before merging is a test for each behavior-changing part of the diff, such that reverting that change causes at least one test to fail.

For example, for line 27 of _cell.py, if I revert [^|\r\n] back to [^|\n], all the current tests still pass. I'd like to have at least one test that specifically exercises the \r case and fails when that change is reverted.

In general, I'd like the new tests to make the changes in this PR regression-protected.

@tacc-tacc

Copy link
Copy Markdown
Contributor Author

@5j9 Are you still writing the tests? I can write some of them if you want.

@5j9

5j9 commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Ah, I think I managed to phrase my previous comment badly enough to make it sound like I was going to write the tests myself. Sorry for the confusion!

I meant that I'd like to see the PR updated with tests covering each behavior-changing part of the diff before merging, and I was hoping you would do it. I might eventually add them myself if you don't, but I just don't know when I'll have the free time to do so.

@tacc-tacc
tacc-tacc force-pushed the main branch 3 times, most recently from 247ffff to bc0ebba Compare August 31, 2026 00:47
@tacc-tacc

tacc-tacc commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@5j9 I added some tests and fixes. Can you accept the PR now? PS: you can't use splitlines in _balanced_quotes_shadow because you lose the linebreaks and we need to keep them untouched, so I changed that for regex.sub.

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.

2 participants