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

New package: AppleHealthParser v1.0.0 #116443

Conversation

JuliaRegistrator
Copy link
Contributor

@JuliaRegistrator JuliaRegistrator commented Oct 2, 2024

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Hello, I am an automated registration bot. I help manage the registration process by checking your registration against a set of AutoMerge guidelines. If all these guidelines are met, this pull request will be merged automatically, completing your registration. It is strongly recommended to follow the guidelines, since otherwise the pull request needs to be manually reviewed and merged by a human.

1. New package registration

Please make sure that you have read the package naming guidelines.

2. AutoMerge Guidelines are all met! ✅

Your new package registration met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period (3 days) has elapsed.

3. To pause or stop registration

If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment.

Tip: You can edit blocking comments to add [noblock] in order to unblock auto-merging.

@carstenbauer
Copy link
Member

carstenbauer commented Oct 2, 2024

Hi @sumant-28, this seems to be only your second attempt to register a package in the General registry. For this reason, and also because you asked on Slack about unit tests, let me leave a few comments about how you could (and probably should) improve your package to make it more suitable for registration in General:

  1. Reusability: The idea behind registering packages is generally to share some reusable functionality that could be useful to (parts of) the Julia community (i.e. a certain function, for example). Scripts are generally not suitable for the General registry. Your package has a main function that isn't customizable at all - i.e. it take any arguments - and thus falls into the script category. For example, for your code to work, the input XML file must be named export.xml and must be located in the current directory. Typically, you would want to relax these constraints and also give main a more meaningful name, i.e. you might want to provide a clean function readxml(filename) as API instead.
  2. Documentation: From reading your README, one has no clue how to use your package. You also have badges at the bottom that are supposed to point to documentation, but don't. At least a minimal "Usage" section would be nice.
  3. Unit Tests: You don't have any. I recommend that you create a export.xml with "reasonable" dummy content, load it in your runtests.jl and test that you get the expected output.
  4. Dependencies: Your are depending on Revise but don't seem to use it in your package.
  5. Compat bounds: You are missing compatibility bounds for the versions of your package's dependencies. (This is why auto-merge failed, see above).

There is probably more to say but these are the most important points. Note that not all of them are mandatory for the registration to go through - we have few strict requirements for packages - but I strongly recommend that you try to implement these suggestions.

[noblock]

UUID: d1ed7681-0bdc-4b3e-a64e-a9a059b3ebfe
Repo: https://github.com/sumant-28/AppleHealthParser.jl.git
Tree: 1122044ed21e54fa8ba62ada326329427c92b02b

Registrator tree SHA: 17aec322677d9b81cdd6b9b9236b09a3f1374c6a
@carstenbauer
Copy link
Member

Hi @sumat-28, did you see my comments above? Especially point 1) would be important to improve to make your package suitable for registration in General, at least in my opinion.

@goerz
Copy link
Member

goerz commented Oct 3, 2024

With some minor fixes, this should be totally fine to register… but it is definitely not yet at the level of maturity required for a 1.0 version. So I would ask to change the version number to 0.1 in addition to considering some of the other comments

@sumant-28
Copy link

Hi @carstenbauer and @goerz . Thanks for your patience and bearing with me. I should note if it isn't obvious that I am trying my best and a lot of this is not easy for me

  1. I agree with your point about reusability. I have added some more functions and now describe two example use cases. One more simple and one less simple. Both use cases are informed by earlier examples of people cleaning the data for export into a form suitable for analytics. I think this fits the minimum requirement for a package because it is not running a script.
  2. I have added some docs using Documenter.jl. However something is not right because the pages are not rendering. Ideally I would have waited a bit longer and tried to troubleshoot before committing but I have already taken up too much time since your initial message and intend on addressing why the documentation is not working. I also intend to build up the pages with a write up and detailed instructions but I want to get on top of the earlier mentioned problem first.
  3. I have added the bare minimum unit test. I had some trouble figuring out how to include an XML file for the tests but now I think it is possible. I hope to build on this just like for the earlier bullet point.
  4. I need to look into this further but I find it tricky because I feel I need to use Revise.jl for the functionality in the package testing. I tried removing it but then my package wouldn't work properly on my machine. I haven't had the time to figure out how to be effective and efficient here.
  5. These have been added. I am following online tutorials. If they do not mention certain implementation details then it is easy for me to make these omissions

I have changed the version to 0.1.0 from the previous 1.0.0. Similar to the earlier point I was simply following a YouTube video which decided to dev packages at that version number

@goerz
Copy link
Member

goerz commented Oct 15, 2024

That package seems fine for initial registration with as v0.1. You can work on more tests and documentation as you continue to develop the package.

I need to use Revise.jl

Revise should never be a dependency of the actual project. At most, it should be a test-dependency, see sumant-28/AppleHealthParser.jl#1

Actually, I recommend installing Revise only into your base environment (the environment that's active when you just run julia without any --project flag). Since environments in Julia are stacked, that allows you to run using Revise in any REPL, even when another project has been activated (like the test environment for AppleHealthParser).

I have added some docs using Documenter.jl. However something is not right because the pages are not rendering.

I tried compiling them locally, with

julia --project=docs
] dev .
include("docs/make.jl")

and they compile just fine. They also seem to build and get deployed in your CI runs: https://github.com/sumant-28/AppleHealthParser.jl/actions/runs/11344011507

You may still have to activate Github Pages for your projects in the project settings.

I also assume you have set up the deploy keys as described in https://documenter.juliadocs.org/stable/man/hosting/. Note that that guide is a little bit out of date: when it is talking about Travis, that information applies pretty much equally to Github Pages.

@sumant-28
Copy link

Could you let me know what is outstanding for me to register the package? I have made some adjustments but some of the checks in the list above do not make a lot of sense to me. It is also not finished from my perspective but I thought it could be registered as a rough 0.1 anyway

@goerz
Copy link
Member

goerz commented Oct 25, 2024

Yeah, should be fine. Just re-do the registration. Since you changed the version number to 0.1, it will create a new PR, and I’ll close this one in favor of the new one.

@goerz
Copy link
Member

goerz commented Oct 25, 2024

Closing in favor of #118023

@goerz goerz closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants