Skip to content

Error reporting: print newline characters escaped#99

Closed
stanislaw wants to merge 1 commit intotextX:masterfrom
stanislaw:stanislaw/newline_reporting
Closed

Error reporting: print newline characters escaped#99
stanislaw wants to merge 1 commit intotextX:masterfrom
stanislaw:stanislaw/newline_reporting

Conversation

@stanislaw
Copy link
Copy Markdown
Contributor

Hello,

This is a contribution from @mettta and @stanislaw that addresses the issue textX/textX#397.

We are not sure if patching StrMatch's _exp_str method is the right way to do it, so using this patch to open a discussion.

When the implementation path is confirmed, we are happy to address the remaining contributor checklist's action points.


Code review checklist

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • Changelog(s) has/have been updated, as needed (see CHANGELOG.md, no need
    to update for typo fixes and such).

@igordejanovic
Copy link
Copy Markdown
Member

Thanks @mettta and @stanislaw for the contribution.

The fix using _exp_str didn't handle two cases: a case with regexes, and a case when the match is the root rule. I added two additional test cases and changed the fix to use eval_attrs in NoMatch.

See this branch. I will merge this to master shortly.

@igordejanovic
Copy link
Copy Markdown
Member

Resolved conflicts and merged to master.

@stanislaw stanislaw deleted the stanislaw/newline_reporting branch July 4, 2023 20:56
@stanislaw
Copy link
Copy Markdown
Contributor Author

Hi @igordejanovic, is there a chance for this small but very useful change to be released already? Thanks.

@igordejanovic
Copy link
Copy Markdown
Member

Hi @stanislaw . Sure. I'll do the release in the following days.

@igordejanovic
Copy link
Copy Markdown
Member

@stanislaw I've just released 2.0.1 with this fix only.

@stanislaw
Copy link
Copy Markdown
Contributor Author

Thank you. I will check how it works with my project.

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