Skip to content

Conversation

@DavidAKopriva
Copy link
Collaborator

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.

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
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.58%. Comparing base (061999f) to head (17ca110).
Report is 1 commits behind head on main.

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              
Flag Coverage Δ
unittests 94.58% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a 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.
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a 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.
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a 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

@DavidAKopriva
Copy link
Collaborator Author

Right. Let's do your doc update PR first, since it's simple. Then let's discuss how to coordinate the HOHQMesh update.

@andrewwinters5000
Copy link
Member

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 NEWS.md to HOHQMesh to inform all users.

DavidAKopriva and others added 8 commits April 21, 2025 09:26
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
@DavidAKopriva
Copy link
Collaborator Author

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.
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a 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)
@andrewwinters5000 andrewwinters5000 merged commit fb5ad93 into main May 13, 2025
9 checks passed
@andrewwinters5000 andrewwinters5000 deleted the variableString branch May 13, 2025 17:04
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.

3 participants