ropensci / osmplotr

Data visualisation using OpenStreetMap objects
https://docs.ropensci.org/osmplotr
132 stars 21 forks source link

Opinionated changes to vignette code using magrittr #24

Closed beemyfriend closed 6 years ago

beemyfriend commented 6 years ago

Added magrittr to the second vignette on 'data maps.'

Made some stylistic changes to the code that, in my opinion, would make it easier to follow.

section 1.3: dat_H and dat_HP were never explicitly created in the knitted vignette. I added them to the beginning of

section 1.7: deleted highways4 as it was never use. IAlso, I don't think connect_highways(highways, bbox) (shown in the knitted vignette) and hwys[[i]] (used on the back end) produce the same result.

codecov-io commented 6 years ago

Codecov Report

Merging #24 into master will decrease coverage by 4.95%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   93.57%   88.61%   -4.96%     
==========================================
  Files          20       20              
  Lines        2194     1880     -314     
==========================================
- Hits         2053     1666     -387     
- Misses        141      214      +73
Impacted Files Coverage Δ
R/make-osm-map.R 72.3% <0%> (-12.88%) :arrow_down:
R/get-bbox.R 87.5% <0%> (-12.5%) :arrow_down:
R/print-osm-map.R 86.27% <0%> (-10.16%) :arrow_down:
R/add-osm-groups.R 87.08% <0%> (-8.87%) :arrow_down:
R/extract-osm-objects.R 68.75% <0%> (-8.83%) :arrow_down:
R/colour-mat.R 93.67% <0%> (-6.33%) :arrow_down:
R/order-lines.R 94.59% <0%> (-5.41%) :arrow_down:
R/connect-highways.R 86.31% <0%> (-4.71%) :arrow_down:
R/add-osm-objects.R 73.07% <0%> (-4.53%) :arrow_down:
R/extract-highways.R 89.18% <0%> (-4.41%) :arrow_down:
... and 8 more

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 48b5c1c...8d024a6. Read the comment docs.

mpadge commented 6 years ago

Thanks for the contribution! I like the re-alignment of all args, it definitely makes it easier to read, and so i'd like to merge the PR after a few changes:

  1. Please revert all the dat_H and dat_HP stuff to the way it was. This relies on the lazy-loaded internal data that are used to construct the vignettes. The code looks like it doesn't use or need them, but creating them during execution makes the code too slow for CRAN to accept it. They are thus necessary behind the scenes, yet I've written the vignette so the reader needs neither to read nor to know about them.
  2. highways4 can indeed be deleted, as can your comments on Lines#704-706. The two now differ because I re-wrote the whole routines, but the plots are nevertheless the same. If you discover any substantial differences, then just open an issue and describe it there.
  3. Could you please remove all of the figures from the PR because they are unchanged.
beemyfriend commented 6 years ago
  1. I reverted most of the backend (pre-knitted) dat_H and dat_HP stuff to the way it was. However, I still think it is necessary to explicitly define those variables in the knitted vignette. I'm not sure what you mean by 'the reader needs neither to read nor to know about them.' The code at sections 1.4, 2, 2.1, 2.2, and 2.3 explicitly mention dat_H and dat_HP. Because of this I switched the london$dat_HP and london$dat_HP of section 1.3 to dat_H and dat_HP so that it matches with the other sections.

  2. I deleted the comments. I don't think there was a substantial difference between the two, but I still noticed it while i was going through the vignette and thought it was worth mentioning.

  3. I removed all of the figures. I actually thought they were necessary to show the vignette's .html file, but I realize now that the .png files are not necessary for the knitted vignette to work.

mpadge commented 6 years ago

cool, thanks. and yeah, you're right with switching from londat$dat_H* to dat_H*, so thanks for that too. Now only one final request: Can you removebasic-maps.html` as well, because that's always auto-generated and doesn't need to be included.

beemyfriend commented 6 years ago

I got rid of the .html file, I had no idea that the .Rmd file could stand alone in the package. That's really cool.

This is actually my first PR so thanks for your patience with me. I'll try to be more helpful in the future.