search

metrumresearchgroup / lastdose

Fast computation of [time since] and [amount of] the most-recent dose in a data set
3 stars 2 forks source link

Requesting Code Review #6

Closed kylebaron closed 4 years ago

kylebaron commented 4 years ago

Primary functionality has been coded and tests written according to issues #1, #2, #3

Requesting code review; I will respond to review items in this pull request and, once the PR is reviewed I will merge into the dev branch.

At that point, I will consider the code reviewed and will start validation procedure per the SDLC.

Dreznel commented 4 years ago

Also it looks like Dev is out of sync with master, should this just be changed to point directly at master?

kylebaron commented 4 years ago

Thanks @Dreznel ; Yes, please do consider all of the code. This was a personal development project that I have brought into Metrum and couldn't think of a way to get all of the code up for review in the pr. Just trying to get it into the SDLC cycle at this point. If there are other issues, please bring them up here. Also happy to follow suggestions for git strategy form making all of the code show up in the diff for the PR.

Dreznel commented 4 years ago

Just confirming that I didn't see any other major issues. Maybe a few of your variable/function names could be more descriptive, but for the time being that comes down to personal preference, and I tend to be very verbose in that regard so I think you're fine.

As for making all the code show up in a diff, I would say just don't worry about it -- we can just review the code without the GitHub tooling on the first pass, and from then on we can focus only one what's updated.

kylebaron commented 4 years ago

Thanks!