Skip to content

Dont pass around re match object #1712

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

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

jayvdb
Copy link

@jayvdb jayvdb commented Oct 23, 2020

Removes dead code and keep the match object within Replacer._replace_match, adding explicit args to the other private methods of Replacer so they are clearer.

Related to #1711

Contribution checklist:

(also see CONTRIBUTING.rst for details)

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation
  • added relevant issue keyword
    in message body
  • added news fragment in changelog folder
    • fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
    • <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
    • if PR has no issue: consider creating one first or change it to the PR number after creating the PR
    • "sign" fragment with "by :user:<your username>"
    • please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by :user:superuser."
    • also see examples
  • added yourself to CONTRIBUTORS (preserving alphabetical order)

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

CI needs to pass

@jayvdb
Copy link
Author

jayvdb commented Oct 23, 2020

The CI failure here is #1699 (comment)

This PR also needs doc amendments IMO, as described on #1711


try:
Copy link
Author

Choose a reason for hiding this comment

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

#1715 is the prefered approach for this chunk.

I'll rebase after that is merged, assuming it is accepted.

Copy link
Author

Choose a reason for hiding this comment

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

Rebased; im ambivalent about whether this is merged or not. Also it feels odd writing a changelog entry for this. I'd be fine with slipping this into another PR to avoid it being a standalone changelog entry.

@jayvdb jayvdb force-pushed the dont-pass-around-re-match-object branch 2 times, most recently from f8e4617 to 0308a15 Compare October 24, 2020 08:44
@@ -0,0 +1 @@
Do not pass around regex match object in Replacer. - by :user:`jayvdb`
Copy link
Member

Choose a reason for hiding this comment

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

Only user-facing changes need changelog.

Copy link
Author

Choose a reason for hiding this comment

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

great. But wont CI fail if this file doesnt exist?

Copy link
Member

Choose a reason for hiding this comment

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

The check will fail but that's not mandatory for merge. You can add some naming of the title too to skip it I think 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Done

@jayvdb jayvdb force-pushed the dont-pass-around-re-match-object branch from 0308a15 to bdd4dac Compare October 24, 2020 13:15
@gaborbernat gaborbernat merged commit 3f9389f into tox-dev:master Oct 26, 2020
ssbarnea pushed a commit to ssbarnea/tox that referenced this pull request Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants