Updates to R site builder to match Stata adodown pkg template

kbjarkefur commented 1 year ago

This PR addresses three things. Use this link to view the diff in the PR as it removes all white space edits that my RStudio is removing: https://github.com/arthur-shaw/adodown/pull/7/files?w=1

pkg file - https://github.com/arthur-shaw/adodown/issues/5

This commit (use this link as this link is set to ignore whitespace in commit) updates the find_file_in_pkg() function to recurse search in subfolders by default but adding the option to only search the exact directore provided. This allows this package to serach for the .pkg file only in the src/ folder which is how an adodown-styled folder is set up.

Deleting the files if they exists

This commit https://github.com/arthur-shaw/adodown/commit/50530eea260b5b8f0b387d8c972e39865c43a64a adds an option in build_site() to delete existing site files before creating new files. This I had to do manually each time when testing build_site() function on my local computer.

Extract meta info from pkg file

This commit https://github.com/arthur-shaw/adodown/commit/eaa5a1889a35c6f10f88629064a21dfebb98451e updates the function get_pkg_metadata() to get meta data on the format used in how this file is created in the adodown template.

Happy to get feedback on this R code as I have less experience in R.

arthur-shaw commented 10 months ago

@kbjarkefur , I'm impressed with your PR as I am sorry it took so long to give it the time/attention it deserved. (Happy to debfrief on th ethings I changed.)

While perhaps overkill, I took inspiration from your better approach to extracting metadata and wrote an extraction function that built on your key insights.

Also, I proposed not only that there be an option to delete the old site directory, but that that option be the default. This seems a sensible default, since it avoids building a site that's an untenable mixture of old and new. This probably won't happen in practice--famous last words--but adopting a default of deleting old files avoids this problem.

There are a few things left unresolved. Unless you have strong opinions, perhaps they could be made into issues for future consideration. Broadly, they turn around how strict we should be on adherence to adodown package file structure and file content. Specifically:

Should we look for images only in the agreed directory (i.e., for now, src/dev/assets) and fail to include an image otherwise?
Should we check that .pkg contains all expected metadata, some metadata (i.e., things passed to site, which isn't everythign yet), or not check at all? If we check and find things missing, how should adodownr react: fail informatively, build the site but warn about missing stuff, etc?
Does find_file_in_pkg() have a reason to exist? Should we try to find (misplaced) files? Or should we point to the relative paths where we expect things (e.g., .pkg is in src, images are in src/dev/assets, etc.)? This function was, in part, meant as a safeguard against our changing our minds as adodown crystalizes.

See #5 and #6 for some past thoughts on that--essentially, to be more restrictive, since we know (and probably want to enforce) adodown structure.

Beyond those questions, does the package in this branch work on your computer too? Attached are some files to replicate my testing: stata_site_test_2024-01-17.zip

arthur-shaw commented 10 months ago

~~NOTE TO SELF: rebase this branch on main to reflect the package's change of name (i.e., 4111842444212795461c034f154db5f74b455f0d)~~

arthur-shaw commented 10 months ago

@kbjarkefur , does this work for you?

kbjarkefur commented 10 months ago

Should we look for images only in the agreed directory (i.e., for now, src/dev/assets) and fail to include an image otherwise?

Yes, I think so. At least for the foreseable future. I think with good documentation this is a very reasonable requirement.

kbjarkefur commented 10 months ago

Should we check that .pkg contains all expected metadata, some metadata (i.e., things passed to site, which isn't everythign yet), or not check at all? If we check and find things missing, how should adodownr react: fail informatively, build the site but warn about missing stuff, etc?

I think the aim should be to be like javascript. To onlt throw errors if really needed and in all other cases try to build the site even if some components are missing. I rather have teams get their site up and running, with some things that are not as expected that they can fix later, than requiring so much to be in a perfect state that teams give up on using adodown.

Warnings are good, but might not be that useful as in GHA (where I assume the vast majorirty of teams will run this) any output is quite hidden if you do not know where to look for.

I think an article with FAQs might be our best solution. With sections like "My logo is not showing" and then bullet points with things to look for.

kbjarkefur commented 10 months ago

Does find_file_in_pkg() have a reason to exist? Should we try to find (misplaced) files? Or should we point to the relative paths where we expect things (e.g., .pkg is in src, images are in src/dev/assets, etc.)? This function was, in part, meant as a safeguard against our changing our minds as adodown crystalizes.

Not sure, I think we can keep if for now in order to try to have as much as possible work. But for anything new we do I think we should use relative path. But lets keep this function in case future features requires it.

kbjarkefur commented 10 months ago

Can you solve the conflicts? I think they came after I merged #12. I think you solve them faster and more accurately than me, but I'm happy to do it if you want me to.

lsms-worldbank / adodownr