Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate config file for deprecated value #7705

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pikomonde
Copy link

Describe the pull request

close: #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

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

Test plan

@pikomonde pikomonde requested a review from unknwon as a code owner March 26, 2024 18:14
@unknwon
Copy link
Member

unknwon commented Mar 27, 2024

Thanks for the PR!

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

@unknwon unknwon added the status: needs followup Need some extra work label Mar 27, 2024
@pikomonde
Copy link
Author

pikomonde commented Mar 27, 2024

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 🤔 . I'm thinking of returning some warnMessages []string for the warnDeprecated function, so it can be asserted.

@unknwon
Copy link
Member

unknwon commented Mar 27, 2024

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

unknwon commented Mar 27, 2024

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.

@pikomonde
Copy link
Author

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 pikomonde closed this Mar 28, 2024
@unknwon
Copy link
Member

unknwon commented Mar 28, 2024

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?

@pikomonde pikomonde reopened this Mar 29, 2024

Choose a reason for hiding this comment

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

Yes i need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs followup Need some extra work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate app.ini about deprecated options
3 participants