lelkin commented 4 years ago

A comment...just for you Matt!

mjskay commented 3 years ago

Thanks for this @lelkin!

I did a code review with suggestions as inline comments (see above/below). I hope it's not too much!

The only other thing I would add is that even though I do prefer using 2 spaces for indentation these days, when I wrote ARTool I used four spaces. Sadly going back and changing this is a bit of a pain since it breaks history, so I haven't bit the bullet on that yet. As a result, could you adjust your code to use 4 spaces instead to be consistent with the rest of the codebase? If you set the project setting to four spaces in RStudio you should be able to automatically fix the indentation using Code -> Reindent Lines. Thanks!

mjskay commented 3 years ago

Ah, two other things you'll want to do @lelkin before much else:

[x] merge from dev onto your branch so that the conflicts above are addressed. This should also cause your branch to have github actions set up properly, which should mean the pull request will automatically do code coverage checks, which leads me to...
[x] make sure your new code does not decrease the overall testing code coverage % of the package.

codecov-io commented 3 years ago

Codecov Report

Merging #22 (2bf0705) into dev (0473441) will increase coverage by 2.82%. The diff coverage is 93.93%.

@@            Coverage Diff             @@
##              dev      #22      +/-   ##
==========================================
+ Coverage   85.60%   88.42%   +2.82%     
==========================================
  Files           6        9       +3     
  Lines         250      380     +130     
==========================================
+ Hits          214      336     +122     
- Misses         36       44       +8

Impacted Files	Coverage Δ
R/art.con.internal.R	`93.33% <93.33%> (ø)`
R/art.R	`84.74% <100.00%> (ø)`
R/art.con.R	`100.00% <100.00%> (ø)`
R/artlm.con.R	`100.00% <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 0473441...2bf0705. Read the comment docs.

mjskay / ARTool

Dev #22

Codecov Report