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

C escapes #174

Closed
wants to merge 2 commits into from
Closed

C escapes #174

wants to merge 2 commits into from

Conversation

thdot
Copy link
Contributor

@thdot thdot commented Oct 27, 2017

A new command line option --c-esacpes is implemented which treats files as if they contain C-style character escapes. So for example "\nHello" is parsed as "hello" instead of "nhello".

What do you think?

thdot added 2 commits October 27, 2017 22:10
A new command line option --c-esacpes is implemented which treats files
as if they contain C-style character escapes. So for example "\nHello"
is parsed as "hello" instead of "nhello".
@codecov-io
Copy link

codecov-io commented Oct 27, 2017

Codecov Report

Merging #174 into master will increase coverage by 0.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   87.81%   88.41%   +0.59%     
==========================================
  Files           2        3       +1     
  Lines         665      656       -9     
  Branches       93       94       +1     
==========================================
- Hits          584      580       -4     
+ Misses         62       58       -4     
+ Partials       19       18       -1
Impacted Files Coverage Δ
codespell_lib/tests/test_basic.py 95.3% <100%> (-0.2%) ⬇️
codespell_lib/_codespell.py 83.33% <100%> (+1.41%) ⬆️
codespell_lib/tests/__init__.py 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e87625e...87b94df. Read the comment docs.

@luzpaz
Copy link
Collaborator

luzpaz commented Oct 27, 2017

I like the idea.

action='store_true', default=False,
help='Treats files as if they contain C-style character '
'escapes. So for example "\\nHello" is parsed as '
'"hello" instead of "nhallo".')
Copy link
Member

Choose a reason for hiding this comment

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

nhallo -> nhello

also \\nHello -> \\nhello or capitalize the other two

And maybe r"\nhello" is clearer than "\\nhello"?

@@ -40,6 +40,7 @@
quiet_level = 0
encodings = ['utf-8', 'iso-8859-1']
word_regex = re.compile(r"[\w\-']+")
c_escape_regex = re.compile(r'\\\w')
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use the standard set of escape sequences?

@thdot thdot mentioned this pull request Nov 12, 2017
@thdot
Copy link
Contributor Author

thdot commented Nov 12, 2017

Thank you @larsoner for the good comment.
The reason why I have not updated this PR so far is that I recognized that the new option doesn't work with the write-changes flag. See my comments for #196

@@ -256,6 +256,15 @@ def test_check_filename():
assert_equal(cs.main('-f', d), 1)


def test_c_escapes():
"""Test c-esacpes option"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Meta spelling error, it should be c-escapes! 😄

@bl-ue
Copy link
Contributor

bl-ue commented Jun 11, 2021

ping @thdot — any progress? If not, we can close this PR.

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.

6 participants