Skip to content

Fix/add support for time#96

Closed
hchienjo wants to merge 6 commits into
masterfrom
fix/add-support-for-time
Closed

Fix/add support for time#96
hchienjo wants to merge 6 commits into
masterfrom
fix/add-support-for-time

Conversation

@hchienjo
Copy link
Copy Markdown
Contributor

@hchienjo hchienjo commented Jun 26, 2025

  1. Add support for time in config.
  2. Update dependencies.
  3. Move to go1.24.1.
  4. Fix linter warnings.

@hchienjo hchienjo self-assigned this Jun 26, 2025
@hchienjo hchienjo force-pushed the fix/add-support-for-time branch from 6fe00f0 to 0d041fa Compare June 26, 2025 07:00
@hchienjo hchienjo requested review from jpcosal and skateinmars June 27, 2025 10:56
@hchienjo hchienjo marked this pull request as ready for review June 27, 2025 10:56
Copy link
Copy Markdown
Member

@philippgille philippgille left a comment

Choose a reason for hiding this comment

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

I think it would make sense to split into distinct PRs, as some changes in this PR are unrelated to each other.

Then for some PRs we could discuss a bit more, while others could already be merged. For example:

  • Fixing linter warnings (e.g. ioutil.TempDir usage): ✅
  • Time: Useful new feature, but different from time.Duration it requires a pointer? Why not handle it the same way? Instead of a nil check people will have to do t.IsZero() and assume the time is not explicitly set to 0001-01-01..., but it's the same with the duration, no? Can both be supported?
  • gopkg.in/yaml.v2 to v3 update: That one's outdated as well, so maybe worth moving to github.com/goccy/go-yaml, if the goal is to remove the usage of outdated dependencies? But can be two steps as well: One PR to bump to v3, another later to migrate to another library
  • Removal of github.com/pkg/errors usage: While I think the consensus is that Go libraries shouldn't return stacktraces and I agree with it, it would be a breaking change for those that expect one to be returned. With confita being a public repo with 117 places importing it, do we want to do this?
  • Go version update: Does any feature require 1.24.1 specifically? Otherwise I'd go with 1.23.0. People with newer toolchains won't be downgraded to it so no downside for them, but people with older tollchains aren't forced through Go's implicit version download without it being necessary

Copy link
Copy Markdown
Contributor

@skateinmars skateinmars left a comment

Choose a reason for hiding this comment

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

Time: Useful new feature, but different from time.Duration it requires a pointer? Why not handle it the same way? 

Agreed, let's be consistent

Go version update

Agreed, the usual strategy is to ensure the last 2 major releases are compatible

I'd also add that changing interface{} with any in the public interfaces could be technically considered a breaking change, so let's not include this for now. We could do a separate "cleanup" PR in anticipation of a major release, that includes the YAML lib change and pkg/errors removal.

@rogpeppe
Copy link
Copy Markdown
Contributor

rogpeppe commented Aug 7, 2025

I'd also add that changing interface{} with any in the public interfaces could be technically considered a breaking change, so let's not include this for now.

Drive-by comment: as any is just a type alias for interface{} it is actually entirely interchangeable so it's OK to change any instance of interface{} to any without breaking strict API compatibility.

@hchienjo
Copy link
Copy Markdown
Contributor Author

Superseded by #99.

@hchienjo hchienjo closed this Mar 18, 2026
@hchienjo hchienjo deleted the fix/add-support-for-time branch March 18, 2026 09:10
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.

4 participants