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

WIP: Add NEWS entry and docs for new BitArray constructors #19042

Merged
merged 1 commit into from
Oct 21, 2016

Conversation

carlobaldassi
Copy link
Member

This is for the new constructors that were introduced in #19018, and the old constructors as well.

I'm issuing a PR because I can't find out how to document the old constructors correctly. The relevant lines are here. For the old constructors, I have two signatures, which are basically BitArray(n1, n2, n3, ...) and BitArray((n1,n2,n3)), but I don't know how to create a single signature for the two (I could only specify one). I also can't just do it like in the docs for Array, because the generic signature BitArray(iters) is used for the "generic iterable" constructor.

cc @MichaelHatherly.

@carlobaldassi carlobaldassi added docs This change adds or pertains to documentation docsystem The documentation building system labels Oct 20, 2016
@MichaelHatherly
Copy link
Member

but I don't know how to create a single signature for the two (I could only specify one)

Usually just using Union to combine the signatures works well enough, but in this case

"""
    BitArray(dims::Integer...)
    BitArray(dims::NTuple{N,Int})

Construct an uninitialized `BitArray` with the given dimensions. Behaves identically
to the [`Array`](:func:`Array`) constructor.
"""
BitArray{N}(::Union{Vararg{Integer}, NTuple{N, Int}})

throws an error

ERROR: TypeError: Union: in parameter, expected Type{T}, got Type{Vararg{Integer,N}}

so I suppose that approach won't work here.

Perhaps the simplest would be to split that docstring into two separate ones that describe the differences between the two constructors, maybe with a doctest each. Or just leave it attached to the Integer... one, since it'll still be discoverable enough I'd think.

(We should also be moving docstrings out of helpdb rather than adding more there.)

@carlobaldassi
Copy link
Member Author

Or just leave it attached to the Integer... one, since it'll still be discoverable enough I'd think.

That was my temporary solution, it's not too bad except it gives the wrong help for help> BitArray((3,3)). Oh well.

(We should also be moving docstrings out of helpdb rather than adding more there.)

I just copied what was done in the other cases since I lost track of how things work exactly. Should I just move the help strings to bitarray.jl?

The new constructors were introduced in #19018.
The docs are actually also for the old constructors.

[ci skip]
@carlobaldassi
Copy link
Member Author

Ok I moved the docs out of helpdb.

@carlobaldassi carlobaldassi merged commit 0033f18 into master Oct 21, 2016
@carlobaldassi carlobaldassi deleted the cb/bitgendoc branch October 21, 2016 00:55
@@ -53,6 +53,9 @@ Library improvements
`countfrom`, `take`, `drop`, `cycle`, `repeated`, `product`, `flatten`, `partition`) have been
moved to the module `Base.Iterators` ([#18839]).

* BitArrays can now be constructed from arbitrary iterables, in particular from generator expressions,
e.g. BitArray(isodd(x) for x = 1:100) [#19018].
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be code highlighted. would really prefer leaving pr's open long enough for them to get reviewed

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, fixed in 3f7891a.

BitArray{N}(dims::NTuple{N,Int}) = BitArray{N}(dims...)
# note: the docs for the two signatures are unified, but only
# the first one is recognized by the help system; it would be nice
# to fix this.
Copy link
Member

Choose a reason for hiding this comment

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

Open an issue for this, rather than just a comment in here? (Otherwise this will probably get forgotten.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #19062.

@@ -342,6 +342,40 @@ Constructors
1 1
1 1

.. function:: BitArray(dims::Integer...)
.. function:: BitArray{N}(dims::NTuple{N,Int})
Copy link
Member

Choose a reason for hiding this comment

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

This should only have a single .. function::, i.e.

.. function:: BitArray(dims::Integer...)
              BitArray{N}(dims::NTuple{N,Int})

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 438d1ea, thanks.

@carlobaldassi carlobaldassi added the needs news A NEWS entry is required for this change label Oct 22, 2016
@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label May 12, 2017
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 docsystem The documentation building system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants