PR #5407 : Sketcher Constrain Contextually tool

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

PR #5407 : Sketcher Constrain Contextually tool

Post by paddle »

Hi there,
I opened two PR a while ago:
#5407 : Constrain Contextually
#5389 : Tool setting Widget (obsolete because 5407 builds on top of it and has fixed a few things.)

Being a new contributor I am not familier with the PR process. And because it's been opened for some time now, I wondered what is the process?
The features can be seen in the announcement forum :
Constrain Contextually: https://forum.freecadweb.org/viewtopic.php?f=9&t=65521
Tool setting Widget : https://forum.freecadweb.org/viewtopic.php?f=9&t=65279

Note it is using OpenBrain's PR #5398
Last edited by paddle on Fri Feb 04, 2022 3:49 pm, edited 1 time in total.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by openBrain »

First, opening a topic here for PR done in GH isn't mandatory. ;)

Process is that your PR will be reviewed and eventually merged (but it can takes time).
Currently it looks like @abdullah (main Sketcher merger) is away. ;)

Edit : if PR #5389 is obsolete, please close it.
GeneFC
Veteran
Posts: 5373
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by GeneFC »

It seems pretty clear from the comments that until very recently there were some problems.

This is a fairly significant change, with the opportunity to do a lot of damage if it is not completely correct.

Patience.

Gene
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by openBrain »

OK, just had a look to PR.
The commit history is insane :
* Duplicated commits
* Commit messages not clear
* Merge commits
* Based on another PR (mine about disabling constraints selection) => Please add a comment about that

As a first thing so your PR is reviewed, you should rebase against master, then go through the whole commit history and clean up everything. ;)
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by paddle »

openBrain wrote: Fri Feb 04, 2022 3:04 pm OK, just had a look to PR.
The commit history is insane :
* Duplicated commits
* Commit messages not clear
* Merge commits
* Based on another PR (mine about disabling constraints selection) => Please add a comment about that

As a first thing so your PR is reviewed, you should rebase against master, then go through the whole commit history and clean up everything. ;)
It is indeed a commit mess. Git is making me quite anxious actually, dunno why but yeah.
The worse is that when you checkout a branch or pull or whatever, then VS is rebuilding the whole project which takes my computer way too long. Which makes my git errors quite annoying.

Ok so that was what I was thinking, Abdullah will apparantly be the merger so it needs to wait until he's back.
GeneFC wrote: Fri Feb 04, 2022 3:02 pm It seems pretty clear from the comments that until very recently there were some problems.

This is a fairly significant change, with the opportunity to do a lot of damage if it is not completely correct.

Patience.

Gene
Yes indeed it has been improved steadily last week. So it makes sens it hasn't been merged yet indeed.
I'm not particularly in a hurry to merge, it's just that I try to make one branche per feature as recommanded, but it's starting to be a mess which makes me anxious. For instance I made the chamfer and arc slot tool first, then the tool widget. Now I want to improve the chamfer and slot tool with Tool Widget as I think it will be much better. But I don't know how to handle it with separate branches without making another mess. I just want to merge them all together to be able to move forward without loosing time thinking about what needs to be done first and that I need to rebuild to work on the copy/paste again and so on. I probably won't be able to keep working full time on FreeCAD for very long. I've got a limited time frame and wanted to make the most out of it.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by openBrain »

paddle wrote: Fri Feb 04, 2022 3:46 pm I probably won't be able to keep working full time on FreeCAD for very long. I've got a limited time frame and wanted to make the most out of it.
OK, but a PR that isn't clean will never get merged...
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by paddle »

openBrain wrote: Fri Feb 04, 2022 3:59 pm
paddle wrote: Fri Feb 04, 2022 3:46 pm I probably won't be able to keep working full time on FreeCAD for very long. I've got a limited time frame and wanted to make the most out of it.
OK, but a PR that isn't clean will never get merged...
Yes I'll try to clean it. Though what should I do about your commit that I cherry-picked?
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by openBrain »

paddle wrote: Fri Feb 04, 2022 4:04 pm Yes I'll try to clean it. Though what should I do about your commit that I cherry-picked?
Just add a note that PR relies on PR #5398
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by paddle »

openBrain wrote: Fri Feb 04, 2022 4:07 pm
paddle wrote: Fri Feb 04, 2022 4:04 pm Yes I'll try to clean it. Though what should I do about your commit that I cherry-picked?
Just add a note that PR relies on PR #5398
I meant in my commit cleanup. If its get squashed with the others it can't be good no? It smells like a conflict storm coming at me :D
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: PR #5407 : Sketcher Constrain Contextually tool

Post by openBrain »

paddle wrote: Fri Feb 04, 2022 4:11 pm I meant in my commit cleanup. If its get squashed with the others it can't be good no? It smells like a conflict storm coming at me :D
Make an interactive rebase and pick my commit at the top of the list. Then you can do whatever you want with other ones. ;)
Post Reply