gogs icon indicating copy to clipboard operation
gogs copied to clipboard

config: validate and print warnings for deprecated options

Open pikomonde opened this issue 2 years ago • 6 comments

Describe the pull request

close: https://github.com/gogs/gogs/issues/6498

A clear and concise description of what the pull request is about, i.e. what problem should be fixed?

Link to the issue:

Checklist

  • [x] I agree to follow the Code of Conduct by submitting this pull request.
  • [x] I have read and acknowledge the Contributing guide.
  • [x] I have added test cases to cover the new code or have provided the test plan.

Test plan

pikomonde avatar Mar 26 '24 18:03 pikomonde

Thanks for the PR!

However, it looks incomplete and missing tests, could you finish it before requesting review?

unknwon avatar Mar 27 '24 02:03 unknwon

Hi @unknwon , yes. Noted, next time, will set a draft PR before asking for the review. Btw, I'm still creating the unit test. But I found some problems, I'm not sure how to test/assert the log.Warn :thinking: . I'm thinking of returning some warnMessages []string for the warnDeprecated function, so it can be asserted.

pikomonde avatar Mar 27 '24 13:03 pikomonde

'm thinking of returning some warnMessages []string for the warnDeprecated function, so it can be asserted.

Yep, that's what I would do and recommend 👍

unknwon avatar Mar 27 '24 15:03 unknwon

I do have more general question about whether we need this PR at all though. The tip of the main is already 0.13.0 that no longer reads those config options, so they're not just deprecated but deleted.

Let's say we do agree it's a QoL improvement, the logic needs to be version aware because for example, [service] section could be repurposed, to have, service-related settings. Or re-introduce the same section-key pair for a different purpose.

All that to say, adding a comment saying something like "Delete this function after 0.14.0 is released." would be good enough.

unknwon avatar Mar 27 '24 15:03 unknwon

Let's say we do agree it's a QoL improvement, the logic needs to be version aware because for example, [service] section could be repurposed, to have, service-related settings. Or re-introduce the same section-key pair for a different purpose.

Oh, okay, it makes sense. Since it will be deleted in next version and can be repurpose later.

All that to say, adding a comment saying something like "Delete this function after 0.14.0 is released." would be good enough

I see, okay, I'll close this PR.

pikomonde avatar Mar 28 '24 14:03 pikomonde

All that to say, adding a comment saying something like "Delete this function after 0.14.0 is released." would be good enough

I see, okay, I'll close this PR.

Hmm, sorry if I was misleading... didn't mean to close this PR. I think what's in the current diff still makes sense to the purpose of this PR.

i.e. just do

// warnDeprecated warns about deprecated configuration sections and options.
+//
+// NOTE: Delete this function after 0.14.0 is released.
func warnDeprecated(cfg *ini.File) []string {

Then finish your TODOs and add tests. Sounds right?

unknwon avatar Mar 28 '24 15:03 unknwon