Append/prepend in file.replace assumes '\n' line endings

[lorengordon]

I noticed this while investigating another issue around the append/prepend functionality. The code uses '\n', which is probably not appropriate on Windows. Should probably change that to os.linesep.

https://github.com/saltstack/salt/blob/2015.5/salt/modules/file.py#L1509

Could alternatively add a linesep parameter that defaults to os.linesep, so people can override if they choose?

[jfindlay]

@lorengordon, I think choosing the line ending from os.linesep is the right way to go. I don't think there is a need to override that with a function parameter, which would have to go into all file execution module functions (and states) that ultimately work with file line endings.

[lorengordon]

@jfindlay Sometimes I work with Unix files with '\n' line endings on a Windows box. If there is no parameter to override a default line separator, it might be nice to attempt to autodetect the line ending. Perhaps just read the first line of the file? (Naive, but easy and probably good for most use cases...)

I think other execution module functions could easily add support for a line separator parameter over time, especially if the logic is separated into a private helper function. I was just referring to a parameter for file.replace. Am I missing something?

[jfindlay]

@lorengordon, I think this could become a big, complex change with many unintended consequences. Autodetection may be a better solution in my opinion. I created #23740 and #24089 at the suggestion of @UtahDave to work around some of these issues and it seems to be working so far.

[lorengordon]

@jfindlay, this is the other issue I was referring to, regarding line endings.

I understand why salt.utils.fopen was modified to require binary mode on windows, but re-reading the io.open() docs, I wonder if it would make sense to use that function instead of the default open(), and change the default back to text mode. io.open supports universal newlines, which is intended to provide cross-platform line-ending support on file handles, while allowing devs to write code assuming \n line endings. The io.open function also takes a newline parameter that can be used to override the default behavior, and that parameter could be exposed via salt.utils.fopen easily.

[jfindlay]

@UtahDave, what is your opinion on this? It seems that io.open is python 2.6, 2.7 and 3 compatible.

[lorengordon]

@jfindlay, found another issue bringing up a similar idea: #14135

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[lorengordon]

Handling of line endings still needs a revamp.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.