search

testdockerwcsim / XTriggerApplication

Other
2 stars 3 forks source link

Feature/sn trigger #36

Closed ast0815 closed 4 years ago

ast0815 commented 4 years ago

Another PR, just to enable discussion of the code.

Based on #30, so it includes all of those commits as well.

tdealtry commented 4 years ago

https://github.com/ast0815/TriggerApplication/compare/feature/sub_sample_times...ast0815:feature/SN-trigger for others interested in reviewing Anyway, looks good apart from the minor comment

brichards64 commented 4 years ago

Ok again discussion should be had in issues and PRs only made when your ready to merge them.

There are also a number of conflicts here. Is this a real PR or can i close it?

ast0815 commented 4 years ago

This is not ready yet. Hence the "WIP" in the title.

Why not discuss code in a PR during development? Having this as a PR instead of an issue makes it easier to actually comment on the code changes.

brichards64 commented 4 years ago

I still prefer issues rather than PRs but iv made a new label for work in progress so i can ignore these. please remove the label when ready

tdealtry commented 4 years ago

Actually I quite like the WIP, because of being able to see & comment on ideas of code changes inline (I know it's not good for package manager "inbox 0" OCD)

brichards64 commented 4 years ago

Yeh i know what you mean Tom.

I think i can make my peace with it now iv implemented the neon turquoise label so when im scanning down the open PRs i can see it easily.

ast0815 commented 4 years ago

Split of two topical PRs ready for merging: #52 #53

ast0815 commented 4 years ago

This is ready now. PRs 68 through 71 split some changes into smaller chunks for easier review. This last bit still depends on WCSim/WCSIm#280.

ast0815 commented 4 years ago

I have split off PR #72. It should not need the double-only WCSim version.

tdealtry commented 4 years ago

@brichards64 can you trigger a build? This should pass it now that WCSim has been updated (and it doesn't depend on the docker base image - Travis does a pull on WCSim). It seems that this is actually required now (thanks for testing @tadoyle!)