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

ENH: Add patient display field for last study date #907

Merged

Conversation

cpinter
Copy link
Member

@cpinter cpinter commented Mar 20, 2020

Many times it is useful to be able to order patients with last study date. This display field has now been added.

Many times it is useful to be able to order patients with last study date. This display field has now been added.
Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this will be a useful feature. The implementation looks good to me.

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

Great, thank you! Could you merge it please (or give me rebase access)?

@lassoan lassoan merged commit 27bc11d into commontk:master Mar 21, 2020
@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

Thanks!

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

By the way: do I have push access to the new Slicer repo or do I need to open a PR to update the hash?

@lassoan
Copy link
Member

lassoan commented Mar 21, 2020

I think you have write access to Slicer/Slicer.

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

I got this:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "CommitCheck" is expected. At least 1 approving review is required by reviewers with write access.
To https://github.com/Slicer/Slicer.git
! [remote rejected]     master -> master (protected branch hook declined)
error: failed to push some refs to 'https://github.com/Slicer/Slicer.git'

@jcfr
Copy link
Member

jcfr commented Mar 21, 2020

@cpinter You have write access but you need to publish a branch to your fork and then create a PR. After that, you could bypass the review process and directly merge because you are core developer. That said, we encourage everyone to go through the review process.

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

Alright, interesting. When there is a non-trivial change, the review process makes a lot of sense. Many times, however, I commit a simple fix, or an update like this. If I need to do the overhead of creating a branch in my fork and push it, then it's just a little bit less administration than going through the normal PR process.

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

That said, it seems I don't have write access after all, at least not like @jcfr described.

image

Please merge if found approppriate
Slicer/Slicer#4754

@lassoan
Copy link
Member

lassoan commented Mar 21, 2020

You can directly push to your fork (you can specify any branch name on the fly) and click on the link that tortoisegit to open a PR, another click to submit the PR. Then anybody else can approve the PR and then merge the PR. We can disable the approval requirement if we find it annoying, but I would try to keep it. PR will be even more useful if more automatic testing is done.

@cpinter
Copy link
Member Author

cpinter commented Mar 21, 2020

Sure, no problem. I just wanted to find out how things work now. Actually the need for approval for everybody is a good thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants