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 docstring for Tar module #173

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

lgoettgens
Copy link
Contributor

Handles the Tar part of JuliaLang/julia#52725.

cc @stevengj

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (166deb3) 97.53% compared to head (0b29591) 97.53%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files           4        4           
  Lines         810      810           
=======================================
  Hits          790      790           
  Misses         20       20           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/src/index.md Outdated Show resolved Hide resolved
@@ -2,6 +2,8 @@
EditURL = "https://github.com/JuliaIO/Tar.jl/blob/master/docs/src/index.md"
```

The `Tar` module provides a simple interface for handling tar archives.

# Tar

```@docs
Copy link
Member

@stevengj stevengj Feb 6, 2024

Choose a reason for hiding this comment

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

I notice that none of these functions are currently exported. In Julia 1.11, they should be declared public, e.g. something like the following to src/Tar.jl:

if VERSION >= v"1.11" # declare public, non-exported API
    eval(Meta.parse("public create, extract, list, rewrite, tree_hash, Header"))
end

since this is apparently the documented API.

I'm not sure if there is a way to avoid eval(Meta.parse(...)) since the public keyword does not parse in earlier Julia versions?

(Otherwise, these symbols aren't included in names(Tar), so their docstrings aren't even being checked by the test below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is out of scope of this PR. I'll reference it in JuliaLang/julia#51335 instead to not forget.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if there is a way to avoid eval(Meta.parse(...)) since the public keyword does not parse in earlier Julia versions?

The only good way to avoid that that I know of is to use Compat.jl's @compat public foo, bar, though this is a fine use of eval(Meta.parse(....)) imo.

Copy link

Choose a reason for hiding this comment

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

Or make a small macro here macro public(v); @assert isexpr(v, :tuple); VERSION ? esc(Expr(:public, v.args...)); nothing; end so that it just needs to add an @ to make it syntactically valid here

Copy link
Contributor

Choose a reason for hiding this comment

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

That macro (with some bug-fixes) is here https://github.com/JuliaLang/Compat.jl/pull/805/files/0c90fae7a869a5f526a4cb13620ea077fc30456e, though it was renamed to @compat public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we get this PR here merged? The discussion about declaring public symbols can be moved to #174 and/or #175. There I already implemented two different approaches

test/runtests.jl Outdated Show resolved Hide resolved
src/Tar.jl Outdated Show resolved Hide resolved
Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
Copy link
Contributor

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

LGTM

@stevengj, is there anything you're waiting for before merge

@stevengj stevengj merged commit 4e9d73a into JuliaIO:master Feb 9, 2024
17 checks passed
@lgoettgens lgoettgens deleted the lg/doc-Tar branch February 9, 2024 07:08
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.

4 participants