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

Added support for mixed line ending in file #45498

Closed
wants to merge 15 commits into
base: 2017.7
from

Conversation

Projects
None yet
5 participants
@t0fik
Contributor

t0fik commented Jan 17, 2018

Adds support for mixed line endings in file.line method.

Test written: No

Not signed

@rallytime rallytime requested a review from terminalmage Jan 19, 2018

@terminalmage

I assume when you say "mixed line ending" you mean windows and unix line endings? If so, this is GTM.

@t0fik

This comment has been minimized.

Contributor

t0fik commented Jan 19, 2018

@terminalmage You assume right. What stands for GTM?

def _split(content):
split_content = []
for linesep_line in content.split(os.linesep):
for content_line in linesep_line.split('\n'):

This comment has been minimized.

@sergeizv

sergeizv Jan 19, 2018

Contributor

This code makes a difference only being executed on platforms with os.linesep != '\n'... Why not use str.splitlines() instead?

This comment has been minimized.

@t0fik

t0fik Jan 19, 2018

Contributor

I'm aware of that.
That specific part of code was tested in file.blockreplace. I do not know how str.splitlines() will behave

@@ -1728,11 +1735,11 @@ def line(path, content=None, match=None, mode=None, location=None,
log.warning('Cannot find text to {0}. File \'{1}\' is empty.'.format(mode, path))
body = ''
elif mode == 'delete':
body = os.linesep.join([line for line in body.split(os.linesep) if line.find(match) < 0])
body = os.linesep.join([line for line in _split(body) if line.find(match) < 0])

This comment has been minimized.

@sergeizv

sergeizv Jan 19, 2018

Contributor

Joining with os.linesep and splitting by something else will lead to all the line breaks replaced in the file...

This comment has been minimized.

@t0fik

t0fik Jan 19, 2018

Contributor

Seems legit to me, to output file with line endings proper to OS.
Converting '\n' to os.linesep is default python behavior for not binary writes.

@sergeizv

This comment has been minimized.

Contributor

sergeizv commented Jan 19, 2018

@t0fik

This comment has been minimized.

Contributor

t0fik commented Jan 19, 2018

@sergeizv What is your proposition, instead converting line endings?
For now that particular function cannot handle line endings, which are not proper for the OS. For example: LF instead CRLF on Windows

@sergeizv

This comment has been minimized.

Contributor

sergeizv commented Jan 20, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Feb 2, 2018

@t0fik and @sergeizv Where did we land on this PR?

@t0fik

This comment has been minimized.

Contributor

t0fik commented Feb 2, 2018

I will rewrite line function to detect line endings. I hadn't time to do it.

t0fik added some commits Feb 2, 2018

@@ -1589,6 +1590,13 @@ def _get_line_indent(src, line, indent):
return ''.join(idt) + line.strip()
def _get_line_ending(src, line):

This comment has been minimized.

@damon-atkins

damon-atkins Feb 3, 2018

Member

This seems to add line ending, rather than get_line_ending consider a different name for the function.
e.g. _add_src_eol(src, line).

This comment has been minimized.

@t0fik

t0fik Feb 4, 2018

Contributor

@damon-atkins I wanted to be consistent with name scheme of _get_line_indent, which adds indentation to line.
IMHO better names to both functions would be _set_line_eol and _set_line_indent

This comment has been minimized.

@damon-atkins

damon-atkins Feb 6, 2018

Member

Good idea, change it

@rallytime rallytime requested a review from terminalmage Feb 6, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Feb 6, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Feb 6, 2018

@@ -1589,6 +1590,13 @@ def _get_line_indent(src, line, indent):
return ''.join(idt) + line.strip()
def _get_line_ending(src, line):

This comment has been minimized.

@damon-atkins

damon-atkins Feb 6, 2018

Member

Good idea, change it

t0fik added some commits Feb 6, 2018

@terminalmage

I'm not comfortable with something that changes this much to be included in a bugfix release, especially with all the changes to file I/O made for Oxygen. In my opinion this should be done in develop.

lstat, path_exists_glob, write, pardir, join, HASHES, HASHES_REVMAP,
comment, uncomment, _add_flags, comment_line, _regex_to_static,
_set_line_indent, apply_template_on_contents, dirname, basename,
list_backups_dir, _assert_occurrence, _starts_till, _set_line_eol)

This comment has been minimized.

@terminalmage

terminalmage Feb 8, 2018

Member

Not sure why you felt the need to indent the entire block here.

This comment has been minimized.

@t0fik

t0fik Feb 28, 2018

Contributor

It was more readable for me, when I was checking imports. I've reverted indentation to original

Add line ending
'''
regex = re.compile('((?<!\r)\n|\r(?!\n)|\r\n)$')
line_ending = regex.search(src).group()

This comment has been minimized.

@terminalmage

terminalmage Feb 8, 2018

Member

What happens on the final line of the file? Not all files end in a newline, and this regex wouldn't match a line without one:

>>> regex = re.compile('((?<!\r)\n|\r(?!\n)|\r\n)$')
>>> regex.search('foo').group()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'group'

This comment has been minimized.

@t0fik

t0fik Feb 28, 2018

Contributor

I'll add guard statement.

except Exception as ex:
raise CommandExecutionError("{0}: '{1}'".format(_get_error_message(ex), regex))
return src and src.group() or regex
return src and src[0] or regex

This comment has been minimized.

@terminalmage

terminalmage Feb 8, 2018

Member

Why is this being done? I don't see this function being referenced in any of the changed code.

This comment has been minimized.

@t0fik

t0fik Feb 28, 2018

Contributor

Function is referenced in code which was not changed, line 1734 (orig 1723). I had to change how file is read (to list instead to string) so I had to modify this function

@rallytime

This comment has been minimized.

Contributor

rallytime commented Feb 28, 2018

@t0fik I am making the rounds on PRs today :)

Did you get a chance to review the comments above? Those will need to be addressed and this change needs to be moved to develop before we can move forward here.

@t0fik

This comment has been minimized.

Contributor

t0fik commented Feb 28, 2018

@rallytime sorry, I was busy. I've reviewed comments and fixed issues.

t0fik added some commits Feb 28, 2018

@t0fik t0fik force-pushed the jdsieci:2017.7_line_mixed_line_ends branch from 5816a85 to bbbb8f1 Feb 28, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Mar 1, 2018

This still needs to be rebased/resubmitted on top of develop.

@t0fik

This comment has been minimized.

Contributor

t0fik commented Mar 1, 2018

I'll resubmit PR to develop on weekend.

@t0fik t0fik referenced this pull request Mar 2, 2018

Merged

Line mixed line ends #46291

@t0fik

This comment has been minimized.

Contributor

t0fik commented Mar 2, 2018

PR resubmited to develop #46291

@t0fik t0fik closed this Mar 2, 2018

@t0fik t0fik deleted the jdsieci:2017.7_line_mixed_line_ends branch Mar 2, 2018

@t0fik t0fik restored the jdsieci:2017.7_line_mixed_line_ends branch Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment