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 more documentation #67

Merged
merged 37 commits into from
Jan 19, 2024
Merged

Add more documentation #67

merged 37 commits into from
Jan 19, 2024

Conversation

annakrystalli
Copy link
Member

@annakrystalli annakrystalli commented Dec 22, 2023

This PR adds:

  • A vignette about validating PRs on GitHub targeted towards hub administrators including details of individual checks.
  • A vignette on validating submissions locally targeted towards teams including details of individual checks.
  • A vignette containing more details on the structure of objects returned by validation functions and how to access more information about individual check objects.
  • A csv detailing all the checks performed across the validation family of functions.

Also intend to add:

  • Add details of model metadata checks to the csv
  • Make appropriate subset of details contained in the csv available in lower level individual function roxygen documentation, e.g. validate_model_data, validate_model_file, validate_model_metadata.
  • Link (segway) vignettes to each other
  • Update NEWS.md including bug fix

To test out, chek out branch locally, install with devtools::install() and build site with pkgdown::build_site().

Docs Preview up here! https://659ebe799507d50a38fa6df3--hubverse-pkgdown-preview.netlify.app/articles/hub-validations-class

@nickreich nickreich self-requested a review January 11, 2024 16:09
Copy link
Contributor

@nickreich nickreich left a comment

Choose a reason for hiding this comment

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

  1. I note that some actions can be turned off when PRs are coming in to clones of the repo. Is this something that we can/should discuss in one of the articles briefly, maybe validate-pr? Noting that I filed a relevant issue over here.
  2. The validate-pr vignette seems good as is, and I think is acceptable. That said, I do think that having some additional content about what the output actually looks like ON GITHUB would be a valuable addition to the vignette. It feels very abstract and "command-line centric" right now, when the advantage of having this set-up is so that we can take advantage of all the nice GUI stuff that comes along with having it run on GitHub Actions. And for a reader who is looking for something about "what will this look like for a modeler end-user" I think this vignette doesn't quite answer that currently. Noting that I think we could merge this PR as is and file a new issue to update the vignette with more of these details/images/links to failing or passing PRs etc...
  3. Is there documentation somewhere for developers that makes it clear that if new canonical validations are added to this package that they should also be added to check_table.csv?

Copy link

github-actions bot commented Jan 12, 2024

@annakrystalli
Copy link
Member Author

annakrystalli commented Jan 12, 2024

  1. I note that some actions can be turned off when PRs are coming in to clones of the repo. Is this something that we can/should discuss in one of the articles briefly, maybe validate-pr? Noting that I filed a relevant issue over here.

Thanks for the issue! Answered there!

  1. The validate-pr vignette seems good as is, and I think is acceptable. That said, I do think that having some additional content about what the output actually looks like ON GITHUB would be a valuable addition to the vignette. It feels very abstract and "command-line centric" right now, when the advantage of having this set-up is so that we can take advantage of all the nice GUI stuff that comes along with having it run on GitHub Actions. And for a reader who is looking for something about "what will this look like for a modeler end-user" I think this vignette doesn't quite answer that currently. Noting that I think we could merge this PR as is and file a new issue to update the vignette with more of these details/images/links to failing or passing PRs etc...

While I can appreciate that some screenshots on GitHub could be useful I generally try to avoid screenshots of code in documentation because they are more cumbersome and time consuming to update (whereas any changes to the output of code will update automatically when rebuilding the site).

Ultimately what we get in the GitHub Action is output on a command line so it will not differ to what is shown in the vignette. Of course, they will need to have some familiarity with GitHub Actions to know where to look for the output and I think this is really where you feel a screenshot would be a useful addition which I do agree with so I've added a single example screenshot to orient folks.

Having said all this, once hubverse-org/hubverse-actions#3 is complete, it's likely that results will be reported as a PR comment as standard. I'll make a note in that issue to ensure documentation is updated accordingly when it's complete.

  1. Is there documentation somewhere for developers that makes it clear that if new canonical validations are added to this package that they should also be added to check_table.csv?

I'll add something now

@annakrystalli annakrystalli changed the title [WIP] Add more documentation Add more documentation Jan 12, 2024
@annakrystalli
Copy link
Member Author

OK! I've now added a note about updating inst/check_table.csv with metadata about the check. Hopefully all your concerns jave been addressed @nickreich ?

@annakrystalli
Copy link
Member Author

I'll wait till @LucieContamin has had a look before merging though. See link above for rendered preview of docs

@nickreich nickreich self-requested a review January 12, 2024 16:12
Copy link
Contributor

@nickreich nickreich left a comment

Choose a reason for hiding this comment

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

Thanks for the responsive comments. Looks good to me.

@LucieContamin
Copy link
Contributor

I am still a little bit unclear about the process for including custom validation function and the link with the new CSV file (if there is any).

Copy link
Contributor

@LucieContamin LucieContamin left a comment

Choose a reason for hiding this comment

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

I might need just a little bit more information on the CSV file and the impact of any changes made on it. I am not sure I totally understand the behavior here.

@annakrystalli
Copy link
Member Author

annakrystalli commented Jan 19, 2024

Hey @LucieContamin !

So the csv file holds metadata about standard checks performed by the validate_* family of functions. It is only used to produce the tables with information about checks in the documentation. It does not have any link to custom functions.

The file is not of any concern to users (i.e. they do not need to know about it) as the information is already presented to users in the documentation. It is mainly developers that need to know about it and update it if they add new checks.

Having said that, your question made me realise (and I believe that's actually what @elray1 was referring to when mentioning functions with further arguments/configurations) that I had not documented the optional functions available through the package. I have now:

  • added those to check_table.csv
  • included tables of optional function details in the section on deploying custom/optional functions
  • made clear in the articles that the info on checks refer to standard default checks and added signposting lto the custom functions vignette for more details on optional/custom functions
  • added more detail to the contributing section for how to update the check_table.csv

Hopefully this addresses any confusion. If not it would be great if you could let me know what is still confusing and importantly, where you think more information might be reuired.

Copy link
Contributor

@LucieContamin LucieContamin 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 additional information, it answers all my questions!

@annakrystalli annakrystalli merged commit 220d6b6 into main Jan 19, 2024
10 checks passed
@annakrystalli annakrystalli deleted the more-docs branch January 19, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
3 participants