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

Add module scoping to unexported functions in docs #823

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Add module scoping to unexported functions in docs #823

merged 1 commit into from
Mar 20, 2021

Conversation

kimlaberinto
Copy link
Contributor

@kimlaberinto kimlaberinto commented Mar 20, 2021

I noticed that some code examples in the docs didn't have module scoping where it needed it.

Referencing the HISTORY.md and #699 , I looked for the un-exported functions in the docs (using the following command snippet).

cd docs/
grep -RIn -Ei "(file\(|filename\(|name\(|get_chunk\(|get_datasets\(|iscontiguous\(|ishdf5\(|ismmappable\(|root\(|readmmap\(|set_dims\!\(|get_access_properties\(|get_create_properties\(|create_external_dataset\()" . -C 3

(the grep line is to look for each function in the list with a parenthesis e.g ismmappable( in the docs folder and display them to console with 3 lines of context.)

I then manually added HDF5 scoping to them I thought appropriate.

I haven't checked if the code works for every single one but I checked a few simple ones like HDF5.immappable and it works. I wanted to touch base with you all (the maintainers) to see if I should double check all the code snippets, or if you think this should be good.

Let me know if there might be more unexported functions that I missed in the docs or if there's anything I should change. Otherwise I think it should be ready to merge/review.

@kimlaberinto
Copy link
Contributor Author

Now I also checked the functions referenced in src/HDF5.jl but nothing came up. So those functions should also be good to go.

grep -RIn -EI "(iscompact\(|ischunked\(|refresh\(|start_swmr_write\(|create_external\()" docs/ -C 3 

So all of these functions listed in src/HDF5.jl I now checked in the docs and should be good. (copied from src/HDF5.jl)

### The following require module scoping ###

# file, filename, name,
# get_chunk, get_datasets,
# get_access_properties, get_create_properties,
# root, readmmap, set_dims!,
# iscontiguous, iscompact, ischunked,
# ishdf5, ismmappable,
# refresh
# start_swmr_write
# create_external, create_external_dataset

Copy link
Contributor

@jmert jmert left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Docs are something I (we) haven't gotten around to improving as much as I'd hoped, so thank you.

Just a quick review looks good to me, and it's not an error to "unnecessarily" scope a call, so I think this is a safe change. (Not saying this is unnecessary, just that not having explicitly checked, it's OK to be wrong.)

@kimlaberinto kimlaberinto changed the title [WIP] Add module scoping to unexported functions in docs Add module scoping to unexported functions in docs Mar 20, 2021
@kimlaberinto
Copy link
Contributor Author

kimlaberinto commented Mar 20, 2021

Thank you! And you're very welcome! Thank you for the Julia package

Feel free to merge :)

@jmert
Copy link
Contributor

jmert commented Mar 20, 2021

@musm It appears the docs preview deploy is errorring but CI didn't fail. Not a barrier to merging this PR, but something we should probably look into.

@musm
Copy link
Member

musm commented Mar 20, 2021

I don't think doc deploys work on forks

@musm musm merged commit 85af395 into JuliaIO:master Mar 20, 2021
@musm
Copy link
Member

musm commented Mar 23, 2021

Whops sorry @jmert that wasn't what you are refering to. Indeed, let me investigate..

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.

3 participants