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.

Sign up for GitHub

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

Jump to bottom

Fix #123. Skip shebang line if present. #130

Closed
wants to merge 3 commits into from
Closed

Fix #123. Skip shebang line if present. #130

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fba760
Fix #123. Skip shebang line if present.
tamere-allo-peter Nov 22, 2021
6e41559
Fix #123 : ignore shebang lines and add an unit test
tamere-allo-peter Nov 23, 2021
dbe4d16
Moved shebang skipping code to fix_code instead of fix_files
tamere-allo-peter Nov 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/yamlfix/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ def fix_files(files: Tuple[TextIOWrapper]) -> Optional[str]:
for file_wrapper in files:
log.debug("Fixing file %s...", file_wrapper.name)
source = file_wrapper.read()
fixed_source = fix_code(source)
if source.startswith("#!"):
# Skip the shebang line if present, leaving it unmodified
eolpos = source.find("\n") + 1
fixed_source = source[:eolpos] + fix_code(source[eolpos:])
else:
fixed_source = fix_code(source)

if file_wrapper.name == "<stdin>":
return fixed_source
Expand Down
20 changes: 19 additions & 1 deletion tests/unit/test_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

import logging
from textwrap import dedent
from io import StringIO

import pytest

from yamlfix.services import fix_code
from yamlfix.services import fix_files, fix_code

true_strings = [
"TRUE",
Expand All @@ -32,6 +33,23 @@
]


def test_fix_files_ignore_shebang() -> None:
"""Ignores shebang lines if present at the beginning of the source."""
source = dedent(
"""\
#! /this/line/should/be/ignored
---
program: yamlfix
"""
)

fakestdin = StringIO(source)
fakestdin.name = "<stdin>"
result = fix_files((fakestdin,))
Copy link
Owner

Choose a reason for hiding this comment

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

fix_files is a wrapper function to run fix_code for all the files it receives as argument. Therefore, if you test fix_code you're testing that part of fix_files.

Given that the setup of the tests of fix_code is more clear, I'd use it instead.

So my suggestion would be to have in this test:

    source = dedent(
        """\
        #! /this/line/should/be/ignored
        ---
        program: yamlfix
        """
    )

    result = fix_code(source)

    assert result == source
    ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I respectfully disagree, because my fix for the shebang line problem wasn't implemented in fix_code, but in fix_files. So calling fix_code from the test code to check my patch, which is to fix_files, will always return a failure.

I think it was more straightforward to patch fix_files than fix_code, and I believe it's also the case wrt issue #135

Copy link
Owner

Choose a reason for hiding this comment

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

I see what you mean. There's another reason why I think it's better to implement this functionality at fix_code level. The program is meant to be used as a library too, and a user may have some string that want's to fix, which doesn't belong from a file. To be able to benefit from your feature, they would need to save that string in a file, and then run fix_files on it.

Please take a moment to think about it, and if you still think it's not worth it, I'll resolve the conversation and merge the PR.

Sorry the PR is taking so long to be resolved, I'm having some busy days and weren't able to review this sooner :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I'll move the fix to fix_code instead of fix_files and modify the test accordingly. I'll try too do this tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for the comprehension and the extra work


assert result == source


def test_fix_code_adds_header() -> None:
"""Adds the --- at the beginning of the source."""
source = "program: yamlfix"
Expand Down