Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor fixes + docstrings #183

Merged
merged 15 commits into from
Jul 24, 2019
Merged

Minor fixes + docstrings #183

merged 15 commits into from
Jul 24, 2019

Conversation

tlienart
Copy link
Collaborator

@tlienart tlienart commented Jul 16, 2019

This PR is mostly inconsequential and the fixes are usually minor happening as I was reading the code:

  • added docstrings where I could, used DocStringExtensions to make this more maintainable
  • used Parameters' @with_kw macro where relevant to automatically get structs constructors with keyword arguments
  • using generators instead of array constructors where possible to avoid unnecessarily allocating stuff (i.e. (e[i] for i in ...) instead of [e[i] for i in ...])
  • irrelevant whitespace changes

I don't think it requires a lot of code review but given there are a lot of small changes I thought it was better to have another pair of eyes quickly have a look in case.

PS: ignore the first bunch of erroneous commits where the tests were not running on my machine due to Julia 1.3 and I basically had to push things to see what was wrong.

@tlienart tlienart changed the title Minorfixes Minor fixes + docstrings Jul 16, 2019
@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #183 into master will decrease coverage by 0.1%.
The diff coverage is 76.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
- Coverage   70.92%   70.81%   -0.11%     
==========================================
  Files          19       19              
  Lines        1410     1398      -12     
==========================================
- Hits         1000      990      -10     
+ Misses        410      408       -2
Impacted Files Coverage Δ
src/MLJ.jl 100% <ø> (ø) ⬆️
src/utilities.jl 86.48% <0%> (+4.43%) ⬆️
src/loading.jl 75.36% <100%> (ø) ⬆️
src/tuning.jl 71.84% <60%> (-0.54%) ⬇️
src/networks.jl 56.3% <72.22%> (-1.2%) ⬇️
src/resampling.jl 80.76% <72.72%> (-2.72%) ⬇️
src/machines.jl 82.75% <77.77%> (-0.3%) ⬇️
src/composites.jl 82.16% <83.33%> (+1.54%) ⬆️
src/metrics.jl 70.58% <86.2%> (+1.2%) ⬆️

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 b55a06f...bb7b762. Read the comment docs.

Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead and push yourself after addressing my minor comments, and addressing merge conflicts. Let me know if you want help with the latter.

@tlienart tlienart merged commit 8cbd64e into master Jul 24, 2019
@ablaom
Copy link
Member

ablaom commented Jul 24, 2019

Thanks!

@ablaom ablaom deleted the minorfixes branch November 29, 2019 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants