FreeCAD Forum

realthunder wrote: ↑Thu Feb 27, 2020 3:25 am I am going through the Expression issue tracker, and find it hard to pinpoint exactly which commit to attribute for solving the issue. And many tickets actually involve multiple issues, that are either resolved, or only partially resolved. Any suggestion on what I shall do with those tickets?

Thankfully you actually don't have to attribute the PR to one specific issue. You could just mention in the git summary that it's related to the pull request but doesn't necessarily have to close the issue. I'd be happy to help. For example, within those tickets that are partially influenced by the commit, You could tell me to close the ticket and create a different one with the more accurate and pinpointed summary etc....

To be clear, what I mean is within the git summary you can type: issue #2220, #4321, #4322,
And mantis will associate the PR with that ticket. (Those are fictitious issues btw). It won't close those tickets unless you write:
closes #2220, #4321, #4322
The tickets you choose to associate (not close} with the PR we can then drill down in to and see if it's worth keeping them open or closing them in favor of a more nuanced issues (since part of it was addressed by the PR)

Kunda1 wrote: ↑Thu Feb 27, 2020 4:08 am You could just mention in the git summary that it's related to the pull request but doesn't necessarily have to close the issue.

What about those commits already in the upstream? For example, the array/map accessing in expression is actually brought in during the big merge.

realthunder wrote: ↑Thu Feb 27, 2020 4:13 am What about those commits already in the upstream? For example, the array/map accessing in expression is actually brought in during the big merge.

We can retroactively associate them, if I understand you correctly .

Soft bump. How's the review process proceeding for this PR?

Kunda1 wrote: ↑Thu Oct 08, 2020 9:52 pm Soft bump. How's the review process proceeding for this PR?

I am not an official reviewer, but I checked this out tonight and functionality-wise it's fantastic. Kudos to realthunder for a great addition that I hope makes it into 0.19. It compiled without an issue, and the functionality I tested didn't give me any problems and behaved exactly as I expected. I love the cyclical tab completion, and I'm happy that it works with spaces in Sketcher constraints. I was surprised that renaming a spreadsheet in the Combo View doesn't rename it "under the hood," but given that, it works well.

Regarding the code itself, I only looked at Gui/ExpressionCompleter.cpp - in a previous discussion a poster had expressed reservations about realthunder's coding style/technique. I don't think that person was talking about "style" in the sense of "what the code looks like" as much as what the code "smells" like. On one hand I agree: first and foremost because the code is neither documented nor particularly self-documenting: all told it's pretty opaque, and I'm reasonably familiar with QCompleter. I suspect that only realthunder is capable of making any changes to it, and if a later developer wants to modify it they will end up rewriting the whole thing (again). That said, I've been reading the FreeCAD forums a lot this week and I think the feeling here is that as an open source project, sometimes it makes sense to just let developers develop. Maybe we pay for it later, but it's worth something to encourage contributors. So in that light, this feature is powerful and useful, and the PR seems compilable and stable. +1 for merging.

chennes wrote: ↑Fri Nov 13, 2020 5:09 am I am not an official reviewer, but I checked this out tonight and functionality-wise it's fantastic. Kudos to realthunder for a great addition that I hope makes it into 0.19. It compiled without an issue, and the functionality I tested didn't give me any problems and behaved exactly as I expected. I love the cyclical tab completion, and I'm happy that it works with spaces in Sketcher constraints. I was surprised that renaming a spreadsheet in the Combo View doesn't rename it "under the hood," but given that, it works well.

Thanks for taking time to test and review the code. The largest chunk of code changes is actually from machine generated scanner/parser code. The actual changes to the syntax files that causes these changes are quite small. But, even excluding those generated codes, this is still a big PR, I know. As the title suggests, the refactoring center around two subjects. The Unit expression and the completer. The completer changes are mostly concentrated in just one file ExpressionCompleter.cpp, and there is a big chunk of code documentation describing the idea behind it. To understand the documentation there, you'll need to be familiar with some FC notions about document, object, internal names, labels, etc. There is also some extension to the original expression reference syntax which are recently introduced before this PR, such as pseudo/local property, sub-object reference. You can find more details here. Note that not all changes described in the linked document are merged. It's just the linked section that's relevant.

The unit expression involves changes in the syntax files and are responsible for all those other changes scattering around.

realthunder wrote: ↑Sun Nov 15, 2020 1:29 am The completer changes are mostly concentrated in just one file ExpressionCompleter.cpp, and there is a big chunk of code documentation describing the idea behind it. To understand the documentation there, you'll need to be familiar with some FC notions about document, object, internal names, labels, etc.

Thanks for the links -- I read the code in ExpressionCompleter.cpp, but I am still learning my way around FreeCAD's code, so I don't have anything productive to say about it. I made a few minor comments on the PR but I'm mostly just poking at it.

realthunder wrote:

regarding expressions and constraint names, have a look at this: https://forum.freecadweb.org/viewtopic. ... 10#p580458
renaming a constrain that had no spaces to include spaces leads to errors on file open, does your PR also adapt the syntax of the expression when such name changes are done?
Can we have expression validation when a constrain/spreadsheet alias are renamed? and when a named constrain used in a expression within the same sketch is set to reference constrain?

I personally would just forbid spaces in constrain names and be done with that , the problem with reference constrains would remain though.

edit: https://forum.freecadweb.org/viewtopic. ... 22#p580522 try to set an expression to a constrain that has a space in the name also fails, is this adressed in this PR? I still think it's better to forbid spaces altogether...

adrianinsaval wrote: ↑Wed Mar 16, 2022 9:15 pm regarding expressions and constraint names, have a look at this: https://forum.freecadweb.org/viewtopic. ... 10#p580458
renaming a constrain that had no spaces to include spaces leads to errors on file open, does your PR also adapt the syntax of the expression when such name changes are done?
Can we have expression validation when a constrain/spreadsheet alias are renamed? and when a named constrain used in a expression within the same sketch is set to reference constrain?

I personally would just forbid spaces in constrain names and be done with that , the problem with reference constrains would remain though.

edit: https://forum.freecadweb.org/viewtopic. ... 22#p580522 try to set an expression to a constrain that has a space in the name also fails, is this adressed in this PR? I still think it's better to forbid spaces altogether...

I just fix the sketch renaming problem here. I'll see if I can port the patch to upstream or to the PR branch here.