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

fix typos in verbose message command #3710

Merged
merged 1 commit into from
Jul 18, 2019
Merged

Conversation

franknarf1
Copy link
Contributor

@franknarf1 franknarf1 commented Jul 18, 2019

With current code, I got a malformed message like...

i. e_id has same type ( character ) as x. e_id . No coercion needed.i. rank has same type ( integer ) as x. rank . No coercion needed.Calculated ad hoc index in 0.010s elapsed (0.000s cpu)

That is: spaces where there shouldn't be any and no newlines between columns in the loop.

Off-topic: I reviewed the Contributing guidelines, but still have more to learn about PRs, testing etc (... planning to in a couple months). I'm making this edit through the github website, since I figure that's simpler than opening a new issue, but am not running test.data.table() etc.

With current code, I got a malformed message like...

> i. e_id has same type ( character ) as x. e_id . No coercion needed.i. rank has same type ( integer ) as x. rank . No coercion needed.Calculated ad hoc index in 0.010s elapsed (0.000s cpu)
@MichaelChirico
Copy link
Member

LGTM, just monitor tests to see if any were depending on that output format

@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #3710 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3710   +/-   ##
======================================
  Coverage    98.3%   98.3%           
======================================
  Files          69      69           
  Lines       13267   13267           
======================================
  Hits        13042   13042           
  Misses        225     225
Impacted Files Coverage Δ
R/bmerge.R 100% <100%> (ø) ⬆️

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 f837d50...ccebeaa. Read the comment docs.

@MichaelChirico
Copy link
Member

still have more to learn about PRs, testing etc (... planning to in a couple months)

re:this, I've been thinking it would be helpful to have something like "Your first PR" that does a walk-through of all the steps of the process. It might be helpful if you could make note of your pain points (even on things completely unrelated to data.table per se such as issues with command-line git)

@mattdowle
Copy link
Member

mattdowle commented Jul 18, 2019

Yes: LGTMT. Looking forward to your PRs @franknarf1!
There's the Minimal-first-time-PR section here :
https://github.com/Rdatatable/data.table/wiki/Contributing#minimal-first-time-pr

@mattdowle mattdowle added this to the 1.12.4 milestone Jul 18, 2019
@mattdowle mattdowle merged commit 1ef7456 into Rdatatable:master Jul 18, 2019
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