Skip to content

Conversation

Byrnetp
Copy link
Contributor

@Byrnetp Byrnetp commented Aug 15, 2024

Synopsis

template.render and template.render_to_str currently return None when the values_needed parameter is set to True, which leads to an error being raised. This fix has the functions return the unrendered template instead. A unit test is added that fails for the old behavior and passes for the new behavior.

Type

  • Bug fix (corrects a known issue)
  • Code maintenance (refactoring, etc. without behavior change)
  • Documentation
  • Enhancement (adds new functionality)
  • Tooling (CI, code-quality, packaging, revision-control, etc.)

Impact

  • This is a breaking change (changes existing functionality)
  • This is a non-breaking change (existing functionality continues to work as expected)

Checklist

  • I have added myself and any co-authors to the PR's Assignees list.
  • I have reviewed the documentation and have made any updates necessitated by this change.

Copy link
Collaborator

@maddenp-cu maddenp-cu left a comment

Choose a reason for hiding this comment

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

Looks good, though I see that the CI formatting check has failed. Probably make format && make test in a development shell and then committing any changes made by that will overcome the failure.

@maddenp-cu
Copy link
Collaborator

Do we need to issue bugfix releases for any previous versions? I suppose we'll need at least a 2.4.1, but if the underlying issue predates the 2.4.0 release, we should probably backport the fix. To test, you could create one-off environments with the latest versions from each major.minor.x series (2.3.3, 2.2.0, 2.1.1, 2.0.2) with e.g. conda create -y -n uwtools-2.3.3 -c ufs-community uwtools=2.3.3 and do a manual test to see if the bug is present at that version. If so, we should consider doing a bugfix release for each series. (It may be that the manual test fails in a different way because the code was different enough that we shouldn't expect the test to work, in which case we can probably ignore that series.) Others on the team can help with the actual releases, but a quick survey would be helpful if you have time to do one.

@Byrnetp
Copy link
Contributor Author

Byrnetp commented Aug 16, 2024

After testing with each an environment for each of the versions you mentioned, I found the same bug exists in versions 2.3.3 and 2.2.0. Versions 2.1.1 and 2.0.2 returned True and log the needed values , but they also required the values parameter to be set or they returned a TypeError, where the later versions didn't require values_src to be specified.

I took a look at the code in each of the version branches in this repository too, and it looks like the bug should exist in 2.1.1 as well: it looks like None is returned when values_needed is True. I'm not sure why in my environment testing True was returned instead.

Copy link
Collaborator

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

LGTM.

@maddenp-cu
Copy link
Collaborator

@Byrnetp Nice work, thank you. I'll prep a 2.4.1 bugfix release and also see about backporting this fix to the earlier ones.

@maddenp-cu maddenp-cu merged commit 1719c6d into ufs-community:main Aug 16, 2024
1 check passed
@Byrnetp Byrnetp deleted the template-render-bugfix branch August 16, 2024 14:30
@maddenp-cu maddenp-cu mentioned this pull request Aug 16, 2024
6 tasks
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