search

paleolimbot / tidyphreeqc

Tidy geochemical modeling using PHREEQC
22 stars 3 forks source link

Write pqi #3

Closed Ignimbrit closed 5 years ago

Ignimbrit commented 5 years ago

Hi there, I've added a couple of new features, the biggest of which is "phr_read_pqi". Additional changes focus on step-by-step expanding the available input helpers and establish the Fix_pH-workaround for constant reaction conditions. Regards Sören

paleolimbot commented 5 years ago

Very cool! I've been wondering if it's possible to read a file cleanly into tidyphreeqc, and what you have here looks very promising! There's a few issues I can see off the bat - if you don't mind having a look at those, I will give a more thorough review after.

First, you haven't organized your git branches in a way that makes it easy for me to merge this. I've fixed it for this PR (you'll have to "pull" the branch in RStudio to get my changes, and after that it should be OK). For next time, if you are going to make more than one PR in a repo (as you have here), you need to configure the original repo as the 'upstream' remote, then "pull" the changes of the upstream into your local master branch (using git pull upstream master in the terminal), and then "push" (in RStudio). Then, switch to the master branch and create a branch (in RStudio). I know it sounds complicated, but eventually you will get used to it.

Second, you've added enough code here that it needs unit tests. I wasn't very good about writing tests when I started tidyphreeqc, but there are a few for the use-db functions. I'd create a file called "test-phreeqc_input_helpers.R" in the "test/testthat" folder and write your tests there. If you're not familiar with testthat, this is a good place to start. I usually start with my example code and use some expectations to make sure the output is correct.

Third, it would help me out if you used the tidyverse style. You do this for the most part, but in particular if you could change 

functions(that = look
          like = this)

to

functions(
  that = look,
  like = this
)

and

like_this #comments

to

# comments
like_this

It will help me read the code better.

Ignimbrit commented 5 years ago

Thanks for the feedback and sorry for messing up the git. I'll see to it that the changes are incorporated and I'll try to read up a bit more on the git workflow. Might take till next weekend though. Dayjob and stuff.

paleolimbot commented 5 years ago

No worries...if you run out of time let me know and I'll fix this and write some tests! It's good stuff and I'm glad not to be writing this alone. I'll be gone for a month starting Thursday so take your time!