Skip to content

Conversation

@42atomys
Copy link

This pull request includes the change discussed on issue #1638, primarily focused on updating dependencies, migrating from slim-sprig to sprout, and refactoring the templater functions.

Changes

  • Updated the Go version from 1.22.0 to 1.23.3 to ensure compatibility with Sprout.
  • Replaced github.com/go-task/slim-sprig/v3 with github.com/go-sprout/sprout.
  • Removed slim-sprig imports and added sprout and its registries.
  • Implemented backward compatibility for original sprig methods.
  • Moved templater functions to a new taskfunc registry to follow Sprout standard.
  • Added comprehensive tests for the new taskfunc package to ensure template functions are correctly tested.
  • Update the documentation to match sprout documentation.

Resolve #1638

@42atomys
Copy link
Author

Hi @andreynering, it's been a long time. I preferred to wait for the v1.0.0 release instead of implementing an unstable version, and now here we are.

I've reflected on all the changes and discovered a few points that need discussion 🙏

  1. Regarding the Go Task Functions, you've implemented a merge function that behaves like mergeOverride in Sprig. This implementation currently overrides the original merge behavior, making merge and mergeOverride functionally the same, although they're based on different libraries. To maintain compatibility, I added an alias to ensure the same behavior is retained, but this seems a bit strange to me:

    sprout.AddAlias(aliasMap, "mergeOverwrite", "merge")

    What would you prefer? Should we keep this alias and drop the packaged merge function? Or should we add a deprecation notice to clarify the difference and guide users to update their templates?

  2. I’ve included all functions from slim-sprig to define which registries to include. As discussed, the crypto package is excluded 😉. Additionally, semver and uuid are not included either. Let me know if you'd like fewer or more functions.

  3. I’ve followed your comments in the code to mark deprecated functions and added other deprecation rules to align with naming conventions:

    func (gtr *GoTaskRegistry) RegisterNotices(notices *[]sprout.FunctionNotice) error {
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("IsSH", "This function is deprecated. Consider removing it from your templates."))
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("FromSlash", "Use `fromSlash` instead."))
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("ToSlash", "Use `toSlash` instead."))
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("ExeExt", "Use `exeExt` instead."))
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("OS", "Use `os` instead."))
    sprout.AddNotice(notices, sprout.NewDeprecatedNotice("ARCH", "Use `arch` instead."))
    return nil
    }

Tip

Deprecation notices are non-breaking changes! They inform users about updates they need to make to their templates before old functions are removed. This ensures no breaking changes between versions. Sprout Notice Documentation

  1. While developing Sprout, I discovered several bugs in functions, including a few panics and memory errors. I’ve fixed them, so some functions might now behave differently (e.g., they returned a panic before but now return nil). This is just an informational note.

  2. To follow Go template conventions, all functions should be errorful. In Sprig, some functions have an error signature (any, error) while others don’t. Similarly, some have a Must version, and some don’t. In Sprout, we decided to enforce error signatures for consistency, meaning all functions return an error when something goes wrong.
    Sprout also provides the option for safe function versions to have a no-error signature (disabled by default). Let me know if you’d like to include this feature.

Thanks for your patience and for taking the time to answer me!

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.

Migrate from sprig (slim-sprig) to sprout?

1 participant