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

Replace NEWS with NEWS.md with more details and examples #2599

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

wader
Copy link
Member

@wader wader commented Jun 2, 2023

Changes mentioned based on picking user facing changes from: git log --oneline -r master...jq-1.6 | grep -v Merge

@wader wader marked this pull request as draft June 2, 2023 16:39
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated
Comment on lines 69 to 137
- Add `--nul-output`/`-0` for null (zero byte) separated output. #1990 @pabs3
```sh
$ jq -n0 '1,2,3' | xxd
00000000: 3100 3200 3300 1.2.3.
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably shouldn't be mentioned? #2535

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is a bit confusing, #1271 was not been merged so jq master seems to still have it. Also the shorthand -0 does not work as jq's argument parser reads it as a negative number i think.

Copy link
Contributor

Choose a reason for hiding this comment

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

If -0 doesn't work, then it shouldn't be documented. And if you're likely to remove the other flag, then even if it technically exists, I'd suggest not announcing it, at least, without some big hazard warnings that it's likely to be removed and pointing to an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now i remember. Short flag -0 do work but only if used in combination with other short flags like in -n0. This is very confusing, so maybe we should only use long flag for this or come up with a different short flag? or fix the argument parsing code somehow?

I do think null separated output can be useful (but can be dangerous with -r) so personally i would like to keep it in some form.

@itchyny what do you think? and i also noticed gojq has no issue with this:

$ gojq -n -0 123
123% # (% is zsh showing there is no new line)
$ jq -n -0 123
-0

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, that's indeed unfortunate.

In the interim, I'd definitely suggest not documenting it, and maybe even removing it until the behavior can be implemented in a less confusing manner.

I personally am a fairly heavy -z / -0 user of various things (e.g. git ls-files -z|xargs -0 or find . -print0|xargs -0 or things where my own code consumes/produces nul delimited content).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to change the PR reference to also include that PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's better to be included in this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes will fix and also steal @itchyny example once im at home

Copy link
Contributor

Choose a reason for hiding this comment

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

Using short flag combination is user's preference, so I think it's better not to use it in the release note. Maybe more example which cannot be archived before would help user know how it is useful?

$ jq -n -0 '1,2,3' | xxd
00000000: 3100 3200 3300                           1.2.3.
# can be used with xargs -0
$ jq -n -0 '"a b c", "d\ne\nf"' | xargs -0 printf '%q\n'
'a b c'
'd'$'\n''e'$'\n''f'
# can be used with read -d ''
$ while IFS= read -r -d '' json; do
>   jq '.name' <<< "$json"
> done < <(jq -n -0 '{name:"a b c"},{name:"d\ne\nf"}')
"a b c"
"d\ne\nf"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, added your examples, changed my 1,2,3 examples to us a,b,c instead, seems better. Also changed to correct say that zero byte is after each output not between.

Could you clarify what you mean by "Using short flag combination is user's preference, so I think it's better not to use it in the release note"? use -n -0 not -n0 in release notes?

@itchyny
Copy link
Contributor

itchyny commented Jul 10, 2023

Maybe we add NEWS.md to dist_doc_DATA even if newer autotools includes this in the dist file? Ref: #2482.

@wader
Copy link
Member Author

wader commented Jul 11, 2023

@itchyny 👍sorry for being a bit slow, on vacation, but will probably have some jq time later in the week

@wader wader force-pushed the news-md branch 6 times, most recently from 32b6209 to 71e8e37 Compare July 27, 2023 07:48
@pkoppstein
Copy link
Contributor

@wader - Looking at the current NEWS.md ...

I think it would be important to organize the news carefully, e.g. along the following lines,
preferably with a TOC and headers:

  1. news about jq command-line options

  2. changes that might affect the behavior of existing jq programs
    a) notable bug fixes
    b) numeric literals
    c) removal of (deprecated) filters
    d) stderr: removal of decorations
    ...

  3. language enhancements

    • else is optional
    • destructuring
    • keywords can be used in $-variable names
      ...
  4. new builtins
    a) builtins to support binary strings
    b) debugging enhancement
    c) abs, ...
    ...

  5. performance enhancements
    (I wouldn't mention run-of-the-mill enhancements such as to transpose)

  6. release builds

  7. other news
    a) quality assurance
    b) web site

  8. Acknowledgements


ALSO:

  • Please note that one of the main reasons for abs is that it won't force the conversion
    to floats that length and fabs actually entail.

@wader
Copy link
Member Author

wader commented Jul 27, 2023

@pkoppstein Thanks, all good input and will update about abs and maybe remove about transpose. You can also add a reviews to suggested wording etc if you want

Yeap some more structure might be good but i also don't think it should not be too structured. I've also tried to sort things in order of "news worthyness for users". Anyway now when most changes with author/PRs etc is collect it's quite quick to restructure things.

NEWS.md Outdated Show resolved Hide resolved
@wader
Copy link
Member Author

wader commented Jul 27, 2023

My idea was also to be able to use the markdown for the 1.7 section more or less as-is for the release on github.

@wader
Copy link
Member Author

wader commented Jul 27, 2023

Should we mention #2750? seems to have quite a lot of dup issues related to it

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@nicowilliams
Copy link
Contributor

Thank you so much for this beautiful NEWS file! I've left a few comments, but it LGTM modulo those comments.

@wader
Copy link
Member Author

wader commented Jul 27, 2023

Thanks @jsoref!

@wader
Copy link
Member Author

wader commented Jul 27, 2023

Thank you so much for this beautiful NEWS file! I've left a few comments, but it LGTM modulo those comments.

Thanks, and i guess we can leave this PR open for now and collect comments

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@wader wader force-pushed the news-md branch 2 times, most recently from 5706898 to 32acdb3 Compare July 28, 2023 09:08
@wader
Copy link
Member Author

wader commented Jul 28, 2023

It would be nice if the mentions of builtins like abs/0 could be links to the documentation? haven't checked if possible, maybe would also require links to be absolute, maybe not good?

@wader wader force-pushed the news-md branch 2 times, most recently from fd078d8 to 47a9f88 Compare July 28, 2023 09:13
@nicowilliams nicowilliams marked this pull request as ready for review July 28, 2023 17:35
@nicowilliams
Copy link
Contributor

Any objections to me merging this right now?

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

Documentation links?

"ab"
"AB"
```
- Adds new builtin `abs` to get absolute value. This was previously possibly using `length` or `fabs` but naming was a bit confusing. @pkoppstein #2767
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Adds new builtin `abs` to get absolute value. This was previously possibly using `length` or `fabs` but naming was a bit confusing. @pkoppstein #2767
- Adds new builtin [`abs`](https://jqlang.github.io/jq/manual/#abs) to get absolute value. This was previously possibly using `length` or `fabs` but naming was a bit confusing. @pkoppstein #2767

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicowilliams @itchyny what do you think? links?

Changes mentioned based on picking user facing changes from:
git log --oneline -r master...jq-1.6 | grep -v Merge
@wader
Copy link
Member Author

wader commented Jul 28, 2023

@jsoref Thanks again, not sure how i got some many of them wrong 🤔

@wader
Copy link
Member Author

wader commented Jul 28, 2023

Any objections to me merging this right now?

I'm ok with merging. Only thing is if we want documentation links?

@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 28, 2023 via email

@nicowilliams nicowilliams merged commit 28af007 into jqlang:master Jul 28, 2023
@nicowilliams
Copy link
Contributor

I'm ok with merging. Only thing is if we want documentation links?

We can always do that in a separate PR :)

Thanks!

@wader wader deleted the news-md branch July 28, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants