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

Document and rationalise worker interface #48

Merged
merged 15 commits into from Jun 3, 2021
Merged

Document and rationalise worker interface #48

merged 15 commits into from Jun 3, 2021

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Jun 2, 2021

  • document the rrq_worker class
  • export the script writer
  • export the "worker from config"

This became necessary when writing the docs

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #48 (4908293) into master (ad0b195) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4908293 differs from pull request most recent head 05817e8. Consider uploading reports for the commit 05817e8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1667      1666    -1     
=========================================
- Hits          1667      1666    -1     
Impacted Files Coverage Δ
R/configure.R 100.00% <ø> (ø)
R/dependencies.R 100.00% <100.00%> (ø)
R/keys.R 100.00% <100.00%> (ø)
R/progress.R 100.00% <100.00%> (ø)
R/worker.R 100.00% <100.00%> (ø)
R/worker_messages.R 100.00% <100.00%> (ø)
R/worker_run.R 100.00% <100.00%> (ø)
R/worker_runner.R 100.00% <100.00%> (ø)
R/worker_spawn.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 ad0b195...05817e8. Read the comment docs.

@richfitz richfitz marked this pull request as ready for review June 2, 2021 14:32
@richfitz richfitz requested a review from r-ash June 2, 2021 14:32
Copy link
Contributor

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

This looks good, small typo I noticed.

Is moving most of the variables in the worker to private just for a cleaner interface to users? Honestly the functions being called with private looks a bit funny to me (and self but to a lesser extent). Feels like if those properties need to be accessed then they should be available as public properties. I see why though, I think none of the functions which take private are called from outside the function? So really just using it as a way to break up the code in the object. Perhaps that suggests though that the object is managing too many concerns and it might be worth breaking it up into several objects. At least that is what you would have to do in something like java but perhaps because we can pass around private it is fine - the code isn't difficult to understand. I think fine as is but maybe something to discuss as a general pattern.

R/worker.R Outdated Show resolved Hide resolved
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
@richfitz richfitz merged commit 939d275 into master Jun 3, 2021
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.

None yet

2 participants