Skip to content

Add docstrings for push, pop, pushfirst, popfirst, insert, deleteat #639

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

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

garrison
Copy link
Contributor

Also tested the new docstrings locally

The main "critique" I could make is that putting -> vec may imply the original vec is returned rather than a modified one. Suggestions welcome for making this more clear.

@garrison
Copy link
Contributor Author

Also, I purposely left out setindex because it should really be documented in Base. However, I just realized that there is actually a slight difference between how setindex behaves here and in Base: in Base (for a Tuple) it will be a no-op if the last index is out of bounds, while here (for a StaticVector) it will throw a BoundsError. So maybe they actually each need a docstring after all.

@coveralls
Copy link

coveralls commented Jul 26, 2019

Coverage Status

Coverage remained the same at 81.681% when pulling b2e7de2 on garrison:jrg/docstrings into be5ae30 on JuliaArrays:master.

@c42f
Copy link
Member

c42f commented Jul 26, 2019

Excellent, thanks! It seems that Base.setindex is not documented nor exported, so I think it would be worth documenting the setindex that we have here.

Regarding -> vec and making it clear that the input vec isn't modified, I suppose you could reword the text slightly. Eg for push:

Return a new vector with item inserted on the end of vec

and similarly for the others.

@c42f c42f added the documentation documentation improvements label Aug 1, 2019
@garrison
Copy link
Contributor Author

garrison commented Aug 6, 2019

Changes made as requested. Please let me know if any other comments.

@fredrikekre
Copy link
Member

putting -> vec may imply the original vec is returned rather than a modified one.

I suggest just dropping that.

@garrison
Copy link
Contributor Author

garrison commented Aug 6, 2019

Done

@c42f
Copy link
Member

c42f commented Aug 7, 2019

Looks great to me, thanks!

@c42f c42f merged commit 41728ad into JuliaArrays:master Aug 7, 2019
@garrison garrison deleted the jrg/docstrings branch August 7, 2019 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants