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

Add newline at the end of formatted notebooks #36

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

weery
Copy link
Contributor

@weery weery commented Feb 10, 2023

Formatting with black adds a newline at the end of each file, which is not a behaviour in blackbricks.

@bsamseth
Copy link
Collaborator

bsamseth commented Feb 10, 2023

Thanks for reporting this and providing a PR!

I've been aware of this for a while, and it has been quite annoying. The reason I haven't just added the newline (like your PR suggests) is that it caused perpetual diffs when running with the --remote flag. It would try to add a newline, but Databricks would not return that newline if we run the command again.

But, prompted by your PR I decided to try to remedy this, and found what I think is a reasonable fix. In short, the format we get out from the API when getting a notebook is with trailing newlines stripped.

Some examples:

  • A notebook with foobar\n comes out from the API as foobar
  • foobar comes out as foobar
  • foobar\n\n comes out as foobar\n

The fix simply involves assuming that the "real" notebook source has a newline at the end, because a remote notebook doesn't terminate with a newline (unless there is a literal actual empty line at the end of the file).

I'm happy with how this works now. The only question is how to release the change. It does constitute a change in the output of all blackbricks invocations, and could be considered a breaking change. On the other hand I'd almost consider it a bug to begin with. I'm not a big fan of a flag, like #38, as I'd want the default to be True for new users, which would defeat the purpose of the flag. I'm leaning towards being strict to semver and consider this a breaking change.

@bsamseth bsamseth merged commit 1ba5094 into inspera:master Feb 10, 2023
@bsamseth
Copy link
Collaborator

The change is now available in a new major version: https://pypi.org/project/blackbricks/ 👍🏻

@weery
Copy link
Contributor Author

weery commented Feb 13, 2023

First off, thanks for a great package.

And secondly; Wow, an extra commit, merge, and package upgrade all within a couple of hours, that's amazing. Thanks for making sure this got merged so quickly, now my data scientist colleagues won't be as mad at me for having a strict CI 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants