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

DYN-5791 applying renaming tool #14844

Merged
merged 3 commits into from
Jan 19, 2024
Merged

DYN-5791 applying renaming tool #14844

merged 3 commits into from
Jan 19, 2024

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

The renaming tool with my fix was applied in the next folder doc\distrib\NodeHelpFiles
Before applying the tool some of the files already renamed were deleted, otherwise the tool throws an exception that the file already exists.
I've updated manually 2 files due that the image name inside the original md file is not the same than the real image (then was not passing the 70 chars limit) then the renaming tool was not processing this case.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

The renaming tool with my fix was applied in the next folder doc\distrib\NodeHelpFiles\

Reviewers

@mjkkirschner
@QilongTang

FYIs

The renaming tool with my fix was applied in the next folder doc\distrib\NodeHelpFiles\
Some of the files already renamed were deleted, otherwise the tool throws an exception that the file already exists.
Updating manually 2 files due that the image name inside the original md file is not the same than the real image (then was not passing the 70 chars limit) then the renaming tool was not processing this case.
Copy link

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang QilongTang added the DNM Do not merge. For PRs. label Jan 17, 2024
@QilongTang QilongTang modified the milestones: 3.0.3, 3.1 Jan 17, 2024
@QilongTang QilongTang removed the DNM Do not merge. For PRs. label Jan 18, 2024
## In Depth
Creates a NurbsSurface with specified control vertices, knots, weights, and U V degrees. There are several restrictions on the data which, if broken, will cause the function to fail and will throw an exception. Degree: Both u- and v- degree should be >= 1 (piecewise-linear spline) and less than 26 (the maximum B-spline basis degree supported by ASM). Weights: All weight values (if supplied) should be strictly positive. Weights smaller than 1e-11 will be rejected and the function will fail. Knots: Both knot vectors should be non-decreasing sequences. Interior knot multiplicity should be no larger than degree + 1 at the start/end knot and degree at an internal knot (this allows surfaces with G1 discontinuities to be represented). Note that non-clamped knot vectors are supported, but will be converted to clamped ones, with the corresponding changes applied to the control point/weight data.
Creates a NurbsSurface with specified control vertices, knots, weights, and U V degrees. There are several restrictions on the data which, if broken, will cause the function to fail and will throw an exception. Degree: Both u- and v- degree should be >= 1 (piecewise-linear spline) and less than 26 (the maximum B-spline basis degree supported by ASM). Weights: All weight values (if supplied) should be strictly positive. Weights smaller than 1e-11 will be rejected and the function will fail. Knots: Both knot vectors should be non-decreasing sequences. Interior knot multiplicity should be no larger than degree 1 at the start/end knot and degree at an internal knot (this allows surfaces with G1 discontinuities to be represented). Note that non-clamped knot vectors are supported, but will be converted to clamped ones, with the corresponding changes applied to the control point/weight data.
Copy link
Contributor

Choose a reason for hiding this comment

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

degree + 1 is now degree 1, expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang good catch, the problem with HttpUtility.UrlDecode method is that the "+" char when is decoded will be replaced by empty space " ".
Then I can see only two options:

  • Modify the renaming tool code so that will be replacing just the "%20" char with space something like string.Replace("%20", " ")
  • The other option is running a script over the doc/distrib/NodeHelpFiles/ folder for replacing the %20 in all the md files content (just when filename > 70 char) specifically in the image part (like the image below) and then apply the renaming tool (it should replace the short image filename correctly). This option imply that we don't modify any code.

I guess the best option is the second one, please let me know your thoughts.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Or should we avoid using + but use word plus in the process to avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang yes, that is the faster option, then I will update the file using the word "plus".
I've tested with all this characters ""!*' ( ) ; : @ & = + $ , / ? % # [ ]" and the "+" is the only one being replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: cef0246


In the example below, a cone is translated along the X-axis. Its distance from the original cone is controlled with a number slider.

In the example below, a cuboid is converted into a Solid Def. Copy the contents of the Watch node to use the Solid Def string representation in another graph.
Copy link
Contributor

@QilongTang QilongTang Jan 18, 2024

Choose a reason for hiding this comment

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

Where do the updates come from? Just curious :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang I noticed that the original Autodesk.DesignScript.Geometry.Geometry.Translate(direction, distance).md file doesn't have the same content than the renamed file M4QGSXM2OJC62OCBK7RPJB4QI2KY3B3N3OAB4I4GHJNAOMXLKKRA.md.

The file Autodesk.DesignScript.Geometry.Geometry.Translate(direction, distance).md was updated by me in the next PR: #14589 but we didn't run the renaming tool so the M4QGSXM2OJC62OCBK7RPJB4QI2KY3B3N3OAB4I4GHJNAOMXLKKRA.md was not updated.

I think we are good to go since this is the expected behavior for all the files not updated (the ones with the hashed name).

image

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with some comments

Added the word "plus" because the renaming tool is replacing the "+" char by empty space.
Copy link

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang
Copy link
Contributor

The reported failure DynamoCoreWpfTests.GraphNodeManagerViewExtensionTests.ContainsEmptyListOrNullTest looks like a sporadic one from recent runs.

@QilongTang QilongTang merged commit c7e42fd into DynamoDS:master Jan 19, 2024
20 of 21 checks passed
QilongTang pushed a commit that referenced this pull request Jan 19, 2024
* DYN-5791 Applying Renaming Tool

The renaming tool with my fix was applied in the next folder doc\distrib\NodeHelpFiles\
Some of the files already renamed were deleted, otherwise the tool throws an exception that the file already exists.

* DYN-5791 Applying Renaming Tool

Updating manually 2 files due that the image name inside the original md file is not the same than the real image (then was not passing the 70 chars limit) then the renaming tool was not processing this case.

* DYN-5791 Applying Renaming Tool Code Review

Added the word "plus" because the renaming tool is replacing the "+" char by empty space.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants