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

Update varinfo() docstring signature #49716

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mgkuhn
Copy link
Contributor

@mgkuhn mgkuhn commented May 10, 2023

The default for m is no longer Main.

@mgkuhn mgkuhn requested a review from rfourquet May 10, 2023 11:36
@mgkuhn mgkuhn added docs This change adds or pertains to documentation backport 1.9 Change should be backported to release-1.9 labels May 10, 2023
@KristofferC KristofferC mentioned this pull request May 19, 2023
51 tasks
@KristofferC KristofferC mentioned this pull request Jun 6, 2023
36 tasks
@KristofferC KristofferC mentioned this pull request Jul 11, 2023
35 tasks
KristofferC added a commit that referenced this pull request Aug 18, 2023
Backported PRs:
- [x] #47782 <!-- Generalize Bool parse method to AbstractString -->
- [x] #48634 <!-- Remove unused "deps" mechanism in internal sorting
keywords [NFC] -->
- [x] #49931 <!-- Lock finalizers' lists at exit -->
- [x] #50064 <!-- Fix numbered prompt with input only with comment -->
- [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized -->
- [x] #50516 <!-- Fix visibility of assert on GCC12/13 -->
- [x] #50635 <!-- `versioninfo()`: include build info and unofficial
warning -->
- [x] #49915 <!-- Revert "Remove number / vector (#44358)" -->
- [x] #50781 <!-- fix `bit_map!` with aliasing -->
- [x] #50845 <!-- fix #50438, use default pool for at-threads -->
- [x] #49031 <!-- Update inference.md -->
- [x] #50289 <!-- Initialize prev_nold and nold in gc_reset_page -->
- [x] #50559 <!-- Expand kwcall lowering positional default check to
vararg -->
- [x] #49582 <!-- Update HISTORY.md for `DelimitedFiles` -->
- [x] #50341 <!-- invokelatest docs should say not exported before 1.9
-->
- [x] #50525 <!-- only check that values are finite in `generic_lufact`
when `check=true` -->
- [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some
cases -->
- [x] #50523 <!-- Avoid generic call in most cases for getproperty -->
- [x] #50860 <!-- Add `Base.get_extension` to docs/API -->
- [x] #50164 <!-- codegen: handle dead code with unsafe_store of FCA
pointers -->
- [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` -->
- [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception
handling. -->

Need manual backport:
- [ ] #48542 <!-- Add docs on task-specific buffering using
multithreading -->
- [ ] #50591 <!-- build: fix various makefile bugs -->


Non-merged PRs with backport label:
- [ ] #50842 <!-- Avoid race conditions with recursive rm -->
- [ ] #50823 <!-- Make ranges more robust with unsigned indexes. -->
- [ ] #50663 <!-- Fix Expr(:loopinfo) codegen -->
- [ ] #49716 <!-- Update varinfo() docstring signature -->
- [ ] #49713 <!-- prevent REPL from erroring in numbered mode in some
situations -->
- [ ] #49573 <!-- Implement jl_cpu_pause on PPC64 -->
- [ ] #48726 <!-- fix macro expansion of property destructuring -->
- [ ] #48642 <!-- Use gc alloc instead of alloc typed in lowering -->
- [ ] #48183 <!-- Don't use pkgimage for package if any includes fall in
tracked path for coverage or alloc tracking -->
- [ ] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [ ] #47615 <!-- Allow threadsafe access to buffer of type inference
profiling trees -->
@ViralBShah
Copy link
Member

Should this get merged (after resolving conflict)?

@mgkuhn
Copy link
Contributor Author

mgkuhn commented Aug 25, 2023

checking conflict with #48860 by @LilithHafner; why not simply wrap such a long signature line?

@LilithHafner
Copy link
Member

LilithHafner commented Aug 25, 2023

The ::Bool type annotations add so little value IMO that they are not worth the screen space. For spaces, I switched from inconsistent spacing to consistent concise spacing. See justification in the OP of that PR

Wrapping the long line seems like a reasonable choice in addition to the abbreviations I added.

For reference, this is what we are "supposed to do" if we follow our own advice:

Always show the signature of a function at the top of the documentation, with a four-space indent so that it is printed as Julia code.

This can be identical to the signature present in the Julia code (like mean(x::AbstractArray)), or a simplified form. Optional arguments should be represented with their default values (i.e. f(x, y=1)) when possible, following the actual Julia syntax. Optional arguments which do not have a default value should be put in brackets (i.e. f(x[, y]) and f(x[, y[, z]])). An alternative solution is to use several lines: one without optional arguments, the other(s) with them. This solution can also be used to document several related methods of a given function. When a function accepts many keyword arguments, only include a placeholder in the signature (i.e. f(x; )), and give the complete list under an # Arguments section (see point 4 below).

https://docs.julialang.org/en/v1/manual/documentation/

The default for `m` is no longer `Main`.
@mgkuhn
Copy link
Contributor Author

mgkuhn commented Aug 26, 2023

Conflict now resolved and keyword-argument list now moved under a new Arguments heading, as suggested in the manual guidance quoted by @LilithHafner.

I also find that Bool parameter lists are sometimes best documented by asking for each a question that can be answered with true or false, therefore I've added some question marks there. (Would that practice be a useful addition to the manual guidance example?)

@mgkuhn mgkuhn removed the request for review from rfourquet August 26, 2023 13:34
@mgkuhn
Copy link
Contributor Author

mgkuhn commented Aug 29, 2023

ready to merge?

@LilithHafner
Copy link
Member

Personally, I'm not a fan of the question marks.

@vtjnash vtjnash removed the backport 1.9 Change should be backported to release-1.9 label Oct 30, 2023
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I am not a fan of the format of questions in docs either. Docs are generally supposed to be in imperative tense ("whether to list stuff" not "also list stuff?")

Also needs a rebase again now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants