Support for RMarkdown and Offline Support #11

ian-flores · 2020-07-23T15:23:12Z

This PR adds support for three use cases:

Summarizing a lesson Use Case - Summarizing a lesson [RMarkdown Support] #8
Linking to a definition Use Case - Linking to a definition [RMarkdown Support] #3
Checking a lesson Use Case - Checking a lesson [Consistency Support] #7

It also adds a GitHub action for a daily download of the package data so that it has the latest version.

…rmd_support merge :w

codecov · 2020-07-23T15:26:31Z

Codecov Report

Merging #11 into master will decrease coverage by 17.17%.
The diff coverage is 6.57%.

@@             Coverage Diff             @@
##           master      #11       +/-   ##
===========================================
- Coverage   72.76%   55.59%   -17.18%     
===========================================
  Files           8       11        +3     
  Lines         224      295       +71     
===========================================
+ Hits          163      164        +1     
- Misses         61      131       +70

Impacted Files	Coverage Δ
R/define.R	`66.66% <0.00%> (ø)`
R/list_glosario_terms.R	`0.00% <0.00%> (ø)`
R/text_definition.R	`0.00% <0.00%> (ø)`
R/validate_document.R	`0.00% <0.00%> (ø)`
R/entry.R	`58.04% <9.67%> (-13.89%)`	⬇️
R/gdef.R	`81.81% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44f6acd...527913c. Read the comment docs.

fmichonneau

I had a quick look and didn't actually test the code (just read it), and I think it all looks good!

I have a suggestion to store the glossary as a plain text YAML file inside the inst/ folder instead of storing it as a binary data package file which should simplify the code.

fmichonneau · 2020-07-23T16:18:57Z

utils/make-glossary.R

@@ -1,16 +1,20 @@
 #!/usr/bin/env Rscript

 # Create .rda version of glossary for R package.


for this, personally, I would only fetch the yml file and put it in a inst/glosario folder instead of making it a binary data file. Then, when reading the glossary locally, you can directly call yaml::read_yaml on system.file("glosario/glossary.yml", package = "glosario"). This would have the advantage of providing people with a plain text version of the file if they wanted to inspect it (also easier to diff if needed), and it would make the code in the package simpler because we could use the same code to parse from the web or from the package directly. But maybe there is a reason you chose this approach?

I would also agree with this, especially if otherwise a binary is being updated every day on the github repo, which will make it grow and grow.

One of the things that I would advocate for is using an application directory instead for offline use, which comes in both R and python flavors (see {rappdirs} and appdirs). This way, people would be able to periodically update their dictionary without needing to update the package itself.

I think we could have both?
Update the content of the inst/ folder daily with GitHub Actions so there is always a fresh copy of the glossary with the package, and create a function "update_local_glossary()` (or something like it) that would update the glossary in the application directory.
This feature seems however non-critical for the first release and could be implemented later.

This feature seems however non-critical for the first release and could be implemented later.

Agreed. My thoughts are largely centered around the patterns I see from R users and reproducibility:

people don't update their packages that often or don't know how to update their packages.

there is no clear indicator of what version of the glossary people have on their machines, so if it defaults to the one in the package, then definitions that exist in the global glossary may be missing in their local version.

if {glosario} is released on CRAN, it will be updated every two months (as per CRAN's policy) at most, but the main glosario repository will be constantly updating.

These situations mean that if Belle installs the package on March 4th and Sebastian installs the package on July 17th, they will have two different versions of the glossary on their machines. Let's say they contribute a few new definitions to the main glossary on July 16th, but neither of them see these definitions on their lesson because the package was only pushed to CRAN on July 1st.

I think it would be good to consider these situations before we release this to CRAN and coordinate with the python implementation so that we can reduce the friction that users see.

These are good points @zkamvar! and made me think that we might want to consider including some type versioning inside the YAML (a hash of the glossary content? a Git SHA1? an increasing counter? a datetime stamp?) so people/machines could compare versions?

I'm going to open a new issue for these discussions

R/validate_document.R

.github/workflows/build_data.yaml

R/text_definition.R

R/gdef.R

…mments in PR #11

ian-flores · 2020-08-03T00:49:32Z

@fmichonneau and @zkamvar I think this is now at a point where we can merge. Agreed or any other changes that you can think of?

zkamvar · 2020-08-03T16:30:11Z

@fmichonneau and @zkamvar I think this is now at a point where we can merge. Agreed or any other changes that you can think of?

Agreed we can merge this to get it moving since RMD support is key. We need to add tests for this, though but that may depend on the outcome of carpentries/glosario#27

R/glosario.R

fmichonneau · 2020-08-04T10:02:09Z

R/glosario.R

 #'
 #' `glosario` provides the infrastructure to create and read multilingual
 #' glossaries. By default, `glosario` providesaccess to a community-curated
 #' glossary hosted by [The
 #' Carpentries](https://github.com/carpentries/glosario).
 #'
 #' @docType package
-#' @name glossary
+#' @name glosario


maybe the name here should be glosario-package?

fmichonneau · 2020-08-04T10:04:02Z

I left 2 small comments, but otherwise, this is good to merge! Thanks Ian!

ian-flores and others added 27 commits July 15, 2020 13:22

Adding list_glosario_terms()

b3f368a

implementing list_glosario_terms with an argument for type

4b8ee29

Use Case mentioned in #3 supported with PDF, HTML and MD support

1afc598

Adding validate_document() to address issue #7

1863b43

Support 3 use cases

f6115dd

Modifying Make

81eaab5

Merging master

c1415d5

Adding GH Action to keep data up-to-date

bd6e3af

Adding R to GH Action

f21dae8

Adding strings to GH Action

2f4d359

Modifying utils/

242fd97

Modifying utils/

a263553

Modifying utils/

bd4dbd3

Pushes glosario to repo

13495f5

Pushes glosario to repo

6bfc6a9

Pushes glosario to repo

933c99f

Pushes glosario to repo

6714229

GH ACTION - Glosario file generated

e773dc9

Final GH Actions Changes

5fde894

Merge branch 'rmd_support' of github.com:carpentries/glosario-r into …

75eb92d

…rmd_support merge :w

Adding the data to the .gitignore

8543f8d

Making build_data run every day instead of on-push

b95b09b

Satisfying the RCMD check gods

fba85e9

Fixing RCMD Check for DEVEL version

e81b1b8

Fixing RCMD Check for DEVEL version

d76d8b9

Test DEVEL on Uvuntu instead

4414cc1

Remove devel

af28c2d

ian-flores requested a review from fmichonneau July 23, 2020 15:23

fmichonneau approved these changes Jul 23, 2020

View reviewed changes

zkamvar reviewed Jul 23, 2020

View reviewed changes

R/text_definition.R Show resolved Hide resolved

zkamvar reviewed Jul 23, 2020

View reviewed changes

R/gdef.R Outdated Show resolved Hide resolved

zkamvar mentioned this pull request Jul 24, 2020

Local sync of glossary.yml carpentries/glosario#27

Open

ian-flores added 2 commits August 2, 2020 05:39

Moving data file to inst/glosario/glossary.yml and adressing other co…

021f97e

…mments in PR #11

Cleaning up repo to merge with main

d6636f0

zkamvar approved these changes Aug 3, 2020

View reviewed changes

fmichonneau reviewed Aug 4, 2020

View reviewed changes

R/glosario.R Outdated Show resolved Hide resolved

fmichonneau reviewed Aug 4, 2020

View reviewed changes

ian-flores mentioned this pull request Aug 8, 2020

Public Method for Accessing Private Attributes in text_definition #13

Open

ian-flores added 2 commits August 8, 2020 07:09

Addressing Francois comments about consistency

fc44a39

Fixing the glossary

527913c

ian-flores merged commit 3039e8c into master Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for RMarkdown and Offline Support #11

Support for RMarkdown and Offline Support #11

ian-flores commented Jul 23, 2020

codecov bot commented Jul 23, 2020 •

edited

Loading

fmichonneau left a comment

fmichonneau Jul 23, 2020

zkamvar Jul 23, 2020

fmichonneau Jul 24, 2020

zkamvar Jul 24, 2020

fmichonneau Jul 24, 2020

zkamvar Jul 24, 2020

ian-flores commented Aug 3, 2020

zkamvar commented Aug 3, 2020

fmichonneau Aug 4, 2020

fmichonneau commented Aug 4, 2020

		@@ -1,16 +1,20 @@
		#!/usr/bin/env Rscript

		# Create .rda version of glossary for R package.

Support for RMarkdown and Offline Support #11

Support for RMarkdown and Offline Support #11

Conversation

ian-flores commented Jul 23, 2020

codecov bot commented Jul 23, 2020 • edited Loading

Codecov Report

fmichonneau left a comment

Choose a reason for hiding this comment

fmichonneau Jul 23, 2020

Choose a reason for hiding this comment

zkamvar Jul 23, 2020

Choose a reason for hiding this comment

fmichonneau Jul 24, 2020

Choose a reason for hiding this comment

zkamvar Jul 24, 2020

Choose a reason for hiding this comment

fmichonneau Jul 24, 2020

Choose a reason for hiding this comment

zkamvar Jul 24, 2020

Choose a reason for hiding this comment

ian-flores commented Aug 3, 2020

zkamvar commented Aug 3, 2020

fmichonneau Aug 4, 2020

Choose a reason for hiding this comment

fmichonneau commented Aug 4, 2020

codecov bot commented Jul 23, 2020 •

edited

Loading