search

chop-dbhi / ehb-datasources

datasource drivers, e.g. REDCap and Nautilus, that can be plugged into the Biorepository Portal
BSD 2-Clause "Simplified" License
2 stars 2 forks source link

issue#29_nautilus_feedback_to_ui #31

Closed seg1129 closed 7 years ago

seg1129 commented 7 years ago

This is adding LIMS error messages to the biorepo-portal UI when there is an issue rendering aliquots.

tjrivera commented 7 years ago

It looks like you're merging master into dev at 1ff18ab. Why is that?

Generally, I would rebase this into a few commits. Right now there are some dupes, and you'd want to structure your commits into logical parts to create a nice linear history so that when reading back its clear what change happened where.

There's a nice tutorial on how to do an interactive rebase (and why) here

Try to find a good balance between cramming a bunch of changes into one commit vs. many many small commits for small one off things. Ideally, they should be logical units of change or components that support the overall feature your branch represents.

seg1129 commented 7 years ago

interesting! I'm not sure why commit says, "merge branch 'master' into dev" at the top of the pull request it says, "seg1129 wants to merge 13 commits into master from dev" and when I review the changed code it is from dev not master. I'll do a rebase instead of merge. Thanks for the tutorial!

tjrivera commented 7 years ago

I would check out the pull request history on https://github.com/chop-dbhi/avocado/ since its extensive and provides good examples

seg1129 commented 7 years ago

so when I move code to dev - I should also rebase there so the commits are cleaned up?

tjrivera commented 7 years ago

It's probably easier to do your rebase locally. I've seen people rebase to better explain a PR and then pending review squash everything down to one commit. I don't know if that's necessary but it would probably be best to pare things down a bit before things go to a review

On Fri, Jun 23, 2017, 1:24 PM SueG notifications@github.com wrote:

so when I move code to dev - I should also rebase there so the commits are cleaned up?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/chop-dbhi/ehb-datasources/pull/31#issuecomment-310725055, or mute the thread https://github.com/notifications/unsubscribe-auth/AEM6KOdm-kwd1hemaTBcTOeBr94_quwqks5sG_S5gaJpZM4ODovE .

seg1129 commented 7 years ago

ok, that makes sense. Thanks for the feedback!