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

[Kconfig style] Fix Kconfig style #3009

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

simbit18
Copy link
Contributor

Summary

Remove spaces from Kconfig files
Add TABs
Replace help => ---help---
Remove extra TABs

Impact

Impact on user: NO

Impact on build: NO

Impact on hardware: NO

Impact on documentation: NO

Impact on security: NO

Impact on compatibility: NO

Testing

local

Remove spaces from Kconfig files
Add TABs
Replace help => ---help---
Remove extra TABs

Signed-off-by: simbit18 <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Feb 24, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements in form, but likely not in substance. While it fills out the required sections, the content is very generic and lacks crucial detail. Here's why it's insufficient and how it can be improved:

Summary Issues:

  • Vague: "Remove spaces from Kconfig files," "Add TABs," etc., are too broad. Which Kconfig files? What specific spaces are being removed (leading, trailing, between keywords)? What is the consistent TAB usage being implemented (e.g., 4 spaces)? Why is "help" being replaced with "---help---"? What constitutes "extra TABs"?
  • Missing Rationale: Why are these changes being made? Is this for code style consistency, improved readability, fixing a bug, or some other reason? The "Why" is the most important part of the summary.

Impact Issues:

  • Likely Incorrect: While these changes might seem like they have no impact, changes to Kconfig files can affect the build process and user experience (e.g., if configurations become unavailable or descriptions are misleading). A more thorough assessment is needed. For example, changing indentation in Kconfig can sometimes break menu structures. Even whitespace changes can impact patching if other changes are based on line/column numbers.
  • Too Concise: Even if there is genuinely no impact, a brief justification (e.g., "No impact on build as these are purely cosmetic changes within Kconfig files") is preferable to just "NO."

Testing Issues:

  • Insufficient Detail: "local" is meaningless. Specify the OS, architecture, board, and any relevant configuration details.
  • Missing Logs: The log sections are empty. Provide actual log output demonstrating the change's behavior before and after. Even if the change is purely cosmetic, showing the relevant portions of the Kconfig file before and after would be helpful.

How to Improve:

  1. Be Specific: Detail the exact changes made, file by file, if necessary.
  2. Explain the "Why": Clearly articulate the rationale for the changes. Even seemingly trivial changes need justification. For example, "Improving Kconfig file consistency by removing trailing spaces and enforcing a 4-space tab indentation."
  3. Thoroughly Assess Impact: Think critically about all potential consequences, however minor. If you conclude there is no impact, explain why.
  4. Provide Detailed Testing Information: List all relevant host and target details. Include build logs, configuration output, or any other information relevant to verifying the changes. Show something changing in the logs, even if it's just whitespace.

By addressing these issues, the PR will be much stronger and more likely to be accepted.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @simbit18 :-)

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the syntax!

@raiden00pl raiden00pl merged commit a0dfd18 into apache:master Feb 25, 2025
37 checks passed
@simbit18 simbit18 deleted the simbit18-kconfig branch February 25, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants