epiverse-trace / blueprints

Software development blueprints for epiverse-trace
https://epiverse-trace.github.io/blueprints
Other
3 stars 3 forks source link

Add package scope page #44

Closed joshwlambert closed 2 months ago

joshwlambert commented 1 year ago

This PR adds a package scope page to the blueprints document. This page contains information around the use of plotting functions, plotting in vignettes and dependencies.

This PR relates to #7.

Bisaloo commented 1 year ago

As a reminder for when I start working on this, this post reflects exactly my experience in other packages, and summarizes what I explained above:

https://mastodon.social/@coolbutuseless@fosstodon.org/110587094854201080

I have been down paths of creating custom plots with lots of arguments. It got painful quickly, as it diverged from what people already understood with {ggplot2}.

Instead I created a few custom geoms/stats, then it was just a case of telling people:

"You know ggplot. Here are our in-house custom geoms. Facets and everything else work the same. Go for it!"

netlify[bot] commented 4 months ago

Deploy Preview for playful-gelato-7892ba ready!

Name Link
Latest commit 9feea8cb15d64c8be6ea98710f77a8ae45d01248
Latest deploy log https://app.netlify.com/sites/playful-gelato-7892ba/deploys/660bc3dabe6e140008afd59e
Deploy Preview https://deploy-preview-44--playful-gelato-7892ba.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

chartgerink commented 4 months ago

Hi - @joshwlambert and I worked on substantially revising this page and re-requested reviews on the new version. Would be great if you could have a look at the new and improved page for the plotting functionality.

PS: I'll fix the markdownlint of course :-)

Bisaloo commented 4 months ago

Before I forget: we need to add @chartgerink in the CONTRIBUTORS.txt list before merging this PR.

chartgerink commented 4 months ago

Thanks @Bisaloo - will revise after @TimTaylor indicates their points as well. I tried to incorporate your explanation without it becoming an exposé on the technical details, but I understand I cut out a bit too much 😅 I'll retry to find the balance.

Bisaloo commented 4 months ago

Thanks @Bisaloo - will revise after @TimTaylor indicates their points as well. I tried to incorporate your explanation without it becoming an exposé on the technical details, but I understand I cut out a bit too much 😅 I'll retry to find the balance.

Yes, that's a fine line. A couple of thoughts on this:

chartgerink commented 3 months ago

I am not sure how to resolve the final issues at the moment. This PR has been open for 10 months, so I started working on it in an attempt to close it out. When a PR/issue is open for this long my experience tells me it is unlikely to be merged/resolved for variable reasons (e.g., context changes, unsatisfying results, open discussion points).

As a result, I now am tending towards cutting the reasoning+alternatives bits to balance out the page (they might also work better in a blog post as @Bisaloo suggested). Once cut, the whole page is <10 lines and as such (potentially) does not really warrant an entire page anymore.

I also notice I get the gist of the technical discussion but writing for it in a balanced manner is more difficult for me 😅 I wonder what best to do at the moment to get this to a mergeable state, because I suspect if we do not do this now this PR will simply go stale and not get merged at all.

TimTaylor commented 3 months ago
chartgerink commented 2 months ago

Okay, it seems like this pull request isn't going anywhere. It's been stale for over a month with no clear path to merging.

To prevent more time being sunk into this, I am closing it until the need arises again to address this issue.