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

Retain trailing newline. #6

Merged
merged 1 commit into from
Jul 5, 2019
Merged

Retain trailing newline. #6

merged 1 commit into from
Jul 5, 2019

Conversation

onelson
Copy link
Owner

@onelson onelson commented Jun 23, 2019

During #3, there was some debate about how newlines should be preserved,
especially with regards to the trailing newline at the end of the
program's output.

While the trailing newline may make the test expectations a little more
ugly, my feeling is it's more correct when compared to what the jq
binary itself produces. A difficult decision to make at this point is
whether or not this diff should be considered a breaking change, bumping
us up to 0.4.x or if it should be a plain bugfix, continuing the current
0.3.x line.

Best as I can tell, callers who are relying on feeding the output into a
parser such as serde will not see any impact. The only callers who may
have been impacted are those who are doing string matching on the raw
string. I'm going to sleep on the matter of which digit to bump the
version by. I may use this as an opportinity to make some other changes
with the opening this creates, perhaps in error handling.

@JeanMertz
Copy link
Contributor

The only remark I could give is that the solution should never follow the desire to do a patch release or minor release.

I don’t think there can be any debate that this is a breaking change, as it changes the output of the public API, regardless if that change actually impacts someone or not. So logically, this would bump the minor version, not the patch version.

I’ve seen some interesting projects being worked on that “take the human aspect out of versioning”. If you test input/output of your program, and one of your changes breaks the test, you’ve got a breaking change and so you release a new major version (unless you are still in a pre-1.0 phase). The problem is that people started associating major version bumps with “marketing releases”, which brings back those human aspects to an otherwise clear-cut decision on what to do after a change.

Anyway, getting a bit pedantic here. Love your crate, and obviously whatever you do is your prerogative. But I wanted to give you my 2ct. 👍

@onelson
Copy link
Owner Author

onelson commented Jun 23, 2019

Yep, I agree.

I'll plan to hold this back for the time being, release 0.3.1 with the interior newline preservation, then couch this until I have a little more to add for the minor bump.

@onelson onelson added this to the v0.4.0 milestone Jun 23, 2019
During #3, there was some debate about how newlines should be preserved,
especially with regards to the trailing newline at the end of the
program's output.

While the trailing newline may make the test expectations a little more
ugly, my feeling is it's more correct when compared to what the `jq`
binary itself produces. A difficult decision to make at this point is
whether or not this diff should be considered a breaking change, bumping
us up to 0.4.x or if it should be a plain bugfix, continuing the current
0.3.x line.

Best as I can tell, callers who are relying on feeding the output into a
parser such as serde will not see any impact. The only callers who may
have been impacted are those who are doing string matching on the raw
string. I'm going to sleep on the matter of which digit to bump the
version by. I may use this as an opportinity to make some other changes
with the opening this creates, perhaps in error handling.
@onelson
Copy link
Owner Author

onelson commented Jul 5, 2019

Rebased to target the crate name change.

@onelson onelson merged commit 24e92ef into master Jul 5, 2019
onelson added a commit that referenced this pull request Jul 6, 2019
onelson added a commit that referenced this pull request Jul 6, 2019
@onelson onelson deleted the reconcile-newlines branch August 1, 2019 06:35
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.

2 participants