Comments (13)
Sure, I can perfectly understand! No worries 😊
I'm going to apply your suggestions
from pre-commit-hooks.
Thank you for the detailed explanation & PR! 😊
I now understand why .expandtabs()
behaves this way:
- input
'foo \t'
-> output'foo '
- input
'fo \t'
-> output'fo '
Alright, I'm going to merge your PR #62
from pre-commit-hooks.
Hi @SteveBarnes-BH!
Interestng, thank you for reporting this.
Would you like to submit a PR with a small unit test covering this case,
in order to fix this bug?
from pre-commit-hooks.
Patch for possible test to show the issue:
--- a/tests/remove_tabs_test.py
+++ b/tests/remove_tabs_test.py
@@ -6,8 +6,9 @@ from pre_commit_hooks.remove_tabs import main as remove_tabs
@pytest.mark.parametrize(
('input_s', 'expected'),
(
- ('foo \t\nbar', 'foo \nbar'),
+ ('foo \t\nbar', 'foo \nbar'),
('bar\n\tbaz\n', 'bar\n baz\n'),
+ ('foo \tfoo2\nbar', 'foo foo2\nbar'),
),
)
def test_remove_tabs(input_s, expected, tmpdir):
from pre-commit-hooks.
Alright, thanks!
I can apply those patchs myself, thank you very much for providing them 😊
But would you like to contribute directly through a Pull Request on this GitHub repo?
Some contributors sometimes like to have their name in the repo commits history,
and I just want to let you decide what you prefer.
from pre-commit-hooks.
Since this is a work account I an raise a bug report or suggestion but contributing code requires a pile of paperwork :-( in advance hence the patch in the issue report ;-) I hope that you understand.
from pre-commit-hooks.
I checked more closely your suggested patch on tests/remove_tabs_test.py
,
and I think that in fact there may be no bug there.
If you consider the input string foo \t\nbar
,
you can see that a whitespace character is present before the \t
,
explaining why the output string is foo \nbar
with 5 whitespace characters,
after running remove-tabs
on it.
from pre-commit-hooks.
But visually foo \tbar
and foo\tbar
will be identical in most editors so the result of foo bar
with 5 spaces will be unexpected. This is due to tab alignment.
from pre-commit-hooks.
Strictly each tab should align the text that follows it to the next whitespaces_count after its position so my solution above, while it will work for many cases is still not perfect.
from pre-commit-hooks.
I think that a much better approach would be:
lines = lines.expandtabs(whitespaces_count)
This should align tabs correctly so that: b' \tA\ttabbed \tstring\twith\tspaces\n\tAn\tother\ttabbed\tline\there!\n'
Gives: b' A tabbed string with spaces\n An other tabbed line here!\n'
Which renders to:
A tabbed string with spaces
An other tabbed line here!
from pre-commit-hooks.
I think I now better understand your point.
.expandtabs()
is a very good suggestion, sadly 'foo \t\nbar'.expandtabs(4)
produces 'foo \nbar'
with 5 whitespace characters
from pre-commit-hooks.
I opened PR #61: what do you think of it @SteveBarnes-BH?
from pre-commit-hooks.
Now that I am on my own machine & time I can go into a little more detail. As an old timer who first learnt to use tabs on a mechanical typewriter, (at school I was made to do secretarial studies in an attempt to improve my spelling & learnt a lot just not spelling), my understanding of what is expected with tabs matches the explanation here.
So if you have a tab size of 4, (the most common for programming), this corresponds to setting tab positions like:
---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---
So a test sample such as:
b'No leading\ttab\n\tleading\ttab\n \tSpace then\tTab\nTabs\tbetween\tevery\tword\nSpace \tthen \ttab \tbetween \tevery \tword.'
Would be visually in your editor or word processor (with fixed width fonts) rendered as below _putting the ruler in as well:
---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---T---
No leading tab
leading tab
Space then Tab
Tabs between every word in the line.
Space then tab between every word in the line.
This is what the built in string/bytes method .expandtabs(4)
gives.
In my personal opinion it is best if the hook leaves the file visually unchanged as, if the originator of the file used a mix of spaces and tabs to align the words in a specific way, then they would expect the output of the hook to look the same.
from pre-commit-hooks.
Related Issues (20)
- Ignore empty line in license HOT 2
- insert_license - Only first year range is replaced in existing license HOT 6
- insert_license - Inserting new license with --use-current-year replaces start years in ranges HOT 3
- Matching multiple license headers? HOT 8
- Rename this repo? HOT 3
- insert-license should not add a LF at the end of the license header file if it does not have one, to prevent an ugly empty separator line HOT 2
- Ignore commit amend message files in .git when running forbid-tabs & co by default HOT 7
- CSS (single line comments) set `//` HOT 1
- insert-license detection does not ignore spaces after comment symbols HOT 5
- chmod hook conflicts with shell chmod in a python virtual env HOT 2
- Is there a way of defining a string in the hooks args instead of --license-filepath HOT 2
- How do I specify the hook to be applied to all files in the repo that ends with .py? HOT 2
- breaking changes in 1.5.2 and 1.5.3 - InvalidManifestError HOT 3
- make `forbid-tabs` and `replace-tabs` accept an exclusion list HOT 4
- Feature Request (insert_license): Use multiline comment for python files HOT 2
- Feature Request (insert_license): Dynamic years in license HOT 2
- An error has occurred: InvalidManifestError HOT 3
- pre-commit fails to install/run due to failures within python-Levenshtein-wheels HOT 1
- Error when using `exclude_types`, but not `exclude` HOT 2
- [Enhancement] Update multiple licence year ends in one run. HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from pre-commit-hooks.