-
Notifications
You must be signed in to change notification settings - Fork 414
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
Added option to display birth and death info in descendant report #1610
Added option to display birth and death info in descendant report #1610
Conversation
This Pull Request now address 2 very different bug reports. @Nick-Hall - How can they be separated? 12758 : [Descendant Report] birth date display needs to be select optional |
This was my mistake, I pushed my quickreport change to my master branch without realizing it would effect this pull request. Perhaps if I reset my master branch to it's previous commit and then pushed that change, this pull request will go back to addressing bug 12758 alone, and PR #1613 will address 12759 alone? I've made a new branch to address 12759 and used that branch to make PR #1613, so it shouldn't be effected if I do this. |
@@ -12,7 +12,7 @@ | |||
# This program is free software; you can redistribute it and/or modify | |||
# it under the terms of the GNU General Public License as published by | |||
# the Free Software Foundation; either version 2 of the License, or | |||
# (at your option) any later version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was removal of this line intentional? If not, it should be reintroduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not intentional, I've gone ahead and reintroduced the line. Thanks for pointing this out!
@uwagura Change looks good! @Nick-Hall This PR is targeted to gramps master, is it too late to target 5.2 release? |
As this change is small and confined to a single plugin, I think we can accept it for the gramps52 branch. |
@uwagura Could you target the maintenance/gramps52 branch for your change? See Changing the base branch of a pull request if needed. Your change will be included in the upcoming release :) |
@hgohel , I've gone ahead and changed the target to maintenance/gramps52, let me know if everything looks good! |
@uwagura Thanks. I thought the rebase to maintenance/gramps52 via GitHub would be smarter, but it brought along commits that are not part of your original change. I don't know if there's a way to resolve this using just GitHub (@Nick-Hall ideas?), so it might be better to revert this PR back to the master branch, and create a new PR for the maintenance/gramps52 so that only your changes are brought in. |
@hgohel yeah, I wasn't sure why it did that. It looks like if I create a new PR, it still brings along the extra commits. Do you think I could fix the issue if I 1.) make a local branch named "lifespan" containing only the relevant commits let me know what you think |
Changing the base branch on GitHub does only that. It doesn't do a rebase of the commits for you. You would also need to do the rebase locally and force push in the manner you are suggesting. |
58247de
to
20ccba7
Compare
I was having trouble with the rebase, so I just pulled the gramps52 branch, manually remade my changes, committed them to a new lifespan branch based off of the gramps52 branch, and then force pushed these changes. It was a bit messier, but I think this pull request should be good now @hgohel . Thank you for your help @QuLogic ! |
Implements #12758.
0c3419d
into
gramps-project:maintenance/gramps52
Made to address Bug #12758. This is a relatively small change, but I made a pull request so that the change and the initial feature request are visible to more experienced developers.