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

Correctly pass newline parameter to built-in open function #478

Merged
merged 11 commits into from
May 13, 2020

Conversation

burkovae
Copy link
Contributor

@burkovae burkovae commented Apr 16, 2020

Motivation

It appears, that newline parameter is missing when smart_open uses the buildin open method.
This one is essential, e.g. when writing csv files under windows

Tests

See test case.

  • Please @maintainers, run the complete test suite, because (i) I lack the infrastructre for testing google and aws stuff (tests are not passing from master branch) and (ii) I'm not able to test against Macs and Unix systems.
  • Please review the parameter position. I put it to the end, so that nothing will break in the first palce. But I may be sensible to put it into the same place as open would expect.
  • I noticed unused parameter kw and removed it as well, no harm done ❤️

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title (hopefully 🙉 )
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass (need support from maintainer here 🥇 )

setup.py Outdated Show resolved Hide resolved
@mpenkov mpenkov self-assigned this Apr 17, 2020
@mpenkov mpenkov force-pushed the ab-explore-csv-bug branch from e0a1a24 to cc0d281 Compare April 17, 2020 01:39
@mpenkov
Copy link
Collaborator

mpenkov commented Apr 17, 2020

I've pushed some changes to polish up your PR. Can you please run the tests on your end and confirm that they pass?

In the future, we'll really need to run tests on Windows as part of our CI. #161

@mpenkov mpenkov changed the title Fix inconsistency with traditional open behaviour of new lines under windows Correctly pass newline parameter to built-in open function Apr 17, 2020
@burkovae
Copy link
Contributor Author

Thank you for the edits. I now remember, why temprorary files are quite nasty under Windows 🔥

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later)

This is the reason, why the test fails atm.

What do you think ?

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 19, 2020

Can you please tweak the test so that it passes? I can't reproduce the problem without Windows, unfortunately. I'd do something like this:

import contextlib

@contextlib.contextmanager
def win_compatible_tempfile(mode):
    with tempfile.NamedTemporaryFile(mode, delete=False) as f:
        try:
            yield f
        finally:
            f.close()
            os.unlink(f.name)

based on this comment bravoserver/bravo#111 (comment)
The reason I use tempfile is because I want to avoid writing to the present working directory. It's poor practice.

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 25, 2020

Please pull from develop and look for a function called named_temporary_file. Use it to work around the problem.

@burkovae
Copy link
Contributor Author

Thank you for your suggestions and improvements. The test is green (at least locally.). Sorry, it took so long to respond.

CHANGELOG.md Outdated
@@ -1,7 +1,11 @@
# Unreleased

<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a merge conflict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oi! Thanks, strangely git status didn't told me.

@mpenkov mpenkov added the bug label Apr 25, 2020
@mpenkov
Copy link
Collaborator

mpenkov commented May 13, 2020

Finally got around to merging this. Good work @burkovae , and congrats on your first PR to smart_open! 🥇

@mpenkov mpenkov merged commit c67afe4 into piskvorky:develop May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output using csv module is inconsistent with traditional open behaviour
3 participants