Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

Implement formatting check for long hyphen #261

Closed
wants to merge 4 commits into from
Closed

Implement formatting check for long hyphen #261

wants to merge 4 commits into from

Conversation

fulminatingmoat
Copy link
Contributor

Not too necessary since missing separator check will detect it as well

@fulminatingmoat fulminatingmoat requested a review from a team as a code owner August 6, 2021 08:27
@TimJentzsch TimJentzsch linked an issue Aug 6, 2021 that may be closed by this pull request
Fix these
./tor/validation/formatting_validation.py:125:1: E302 expected 2 blank lines, found 1
./tor/validation/formatting_validation.py:130:1: W293 blank line contains whitespace
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

The implementation itself looks good, however we also need to define the response string for this formatting issue in tor/strings/en_US.yml. For examples you can take a look at the other formatting issues.

test/validation/test_formatting_validation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

We still need to define the corresponding strings for the bot response.

@TimJentzsch
Copy link
Contributor

Here we still need to define the bot response for the volunteers, you can take https://github.com/GrafeasGroup/tor/blob/main/tor/strings/en_US.yml#L209-L319 as examples :)

"""
return (
FormattingIssue.AUTOGENERATED_LONG_HYPHEN
if AUTOGENERATED_LONG_HYPHEN_PATTERN.search(transcription) is not None
Copy link
Member

Choose a reason for hiding this comment

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

Why not drop the regex and do a simple x in y check?

Suggested change
if AUTOGENERATED_LONG_HYPHEN_PATTERN.search(transcription) is not None
if "\u2014" in transcription # \u2014 == em dash == "long hyphen"

],
)
def test_check_for_autogenerated_long_hyphen(test_input: str, should_match: bool) -> None:
"""Test if autogenerated long hyphens are detected."""
Copy link
Member

Choose a reason for hiding this comment

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

The long hyphen isn't always auto-generated, so perhaps "accidental" would be a better descriptor than "autogenerated"?

Side note: if settings for "smart quotes" (and the like) are enabled on macOS, it automatically replaces -- with there too. Not just mobile. 😄

@TheLonelyGhost
Copy link
Member

No comments and no meaningful code updates in months. Closing this since it seems pretty stale to me. Feel free to reopen if the mood strikes to pick this up again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add formatting check for autogenerated long hyphens
4 participants