-
Notifications
You must be signed in to change notification settings - Fork 1
Variable string #60
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
Variable string #60
Conversation
Remove the "requestedLength" arguments and use allocatable strings now that these are available.
The readme is updated. Add users guide in markdown format and remove latex version.
Remove more dependences on fixed length character variables.
Add links to examples. Add info about optData functionality to the documentation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
+ Coverage 94.50% 94.58% +0.07%
==========================================
Files 32 32
Lines 2820 2860 +40
==========================================
+ Hits 2665 2705 +40
Misses 155 155
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andrewwinters5000
left a comment
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.
The code simplifications look good. My comments are concentrated into the new User Guide. Overall, converting the LaTeX documentation into markdown is very good. There seem to be some slight spacing inconsistencies across the example code blocks.
Align code blocks.
More code formatting. Fix link to Apple.
Caught a couple more alignment errors.
andrewwinters5000
left a comment
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.
Everything looks good, just one last question.
Fix an escape for a single underscore.
andrewwinters5000
left a comment
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.
LGTM! This is good to merge. We just need to coordinate with analogous changes in HOHQMesh for the breaking change
|
Right. Let's do your doc update PR first, since it's simple. Then let's discuss how to coordinate the HOHQMesh update. |
|
Yes, I think that is the best order of operations. Then we can make a new release of HOHQMesh that contains the FToL changes and add an appropriate |
Fix code examples formatting.
Fix code example object names.
Add back the legacy calls to stringValue and stringValueForKey so that the changes in this version are non-breaking. Those functions are now generic with either the requested length or not. Going forward, one should use the versions without the requested length and use allocated strings.
Change the returned character length in the legacy function to what was originally there to get rid of the string truncation warning issue.
Increase coverage in stringValueForKey
Fix logical return and add test for it.
cover more stringValue tests
|
Legacy functions with requestedLength have been added back in so that code already using FTOL does not have to be modified. But one may want to use the new allocatable steering versions moving forward. |
A news page hanging off of the readme page is added for the latest and future news about changes.
andrewwinters5000
left a comment
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.
LGTM! I just left one question for clarification.
Just change the comments to reflect that stringValueA and stringValueR are overloaded as stringValue() and that the latter is deprecated but will still work with stringValue(requestedLength)
When FTOL was first written, gfortran did not have allocatable strings implemented yet, so a workaround was used where a requested length parameter would set the string length for returns from stringValue. This is now removed (finally) since fortran includes the functionality now.