-
Notifications
You must be signed in to change notification settings - Fork 8.1k
doc: update KConfig style guidelines #95132
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
base: main
Are you sure you want to change the base?
Conversation
Hello @ParaZera, and thank you very much for your first pull request to the Zephyr project! |
8193d69
to
d381ed0
Compare
78ff9dc
to
f98ad96
Compare
doc/contribute/style/kconfig.rst
Outdated
|
||
.. code-block:: kconfig | ||
# symbol to enable/disable the feature = "enabling symbol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# symbol to enable/disable the feature = "enabling symbol" | |
# Symbol to enable/disable the feature ("enabling symbol") |
Adjust everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParaZera you might have missed this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I missed it 😅 . Now, every comment should be updated as you requested.
381e145
to
7e76895
Compare
7e76895
to
6a629ae
Compare
@@ -0,0 +1,27 @@ | |||
# Symbol to enable/disable the feature ("enabling symbol") | |||
menuconfig ADC_ADI_AD4114 | |||
bool "AD4114 (ADI) ADC driver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the abbreviated manufacturer mentioned here. I'd write it in full, or leave it out.
bool "AD4114 (ADI) ADC driver" | |
bool "AD4114 (Analog Devices, Inc) ADC driver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale behind using the abbreviated form is that it might be easier to search for the manufacturer in the UI.
I updated the manufacturer names in the examples to match common occurrences in the repository.
6a629ae
to
cd4e944
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't improve things
config ADC_ADI_AD405X | ||
bool "AD405X (Analog Devices, Inc) ADC driver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't even right drivers/adc/Kconfig.ad405x
has:
config ADC_ADI_AD405X | |
bool "AD405X (Analog Devices, Inc) ADC driver" | |
config ADC_AD405X | |
bool "AD405X ADC driver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it different from the real KConfig file right now. But the PR is trying to set up guidelines as requested in issue ##7272 to have a more consistent way of naming Kconfig symbols.
Can you be more specific on what things you feel would not be improved by this guideline so I can better address it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an improvement, the original is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue against this because:
- This circumvents potential naming conflicts as a manufacturer will not release two devices under the same name while different manufacturers might at the cost of minimally longer Kconfig symbols
- Especially in the prompt it enables better search capabilities (because more information is available)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This circumvents potential naming conflicts as a manufacturer will not release two devices under the same name while different manufacturers might at the cost of minimally longer Kconfig symbols
this is false though? Let's take the 34063
which is an old DC/DC converter, or even the humble 1n4001
diode, there are many different manufactures for both of these, so to claim that a device will not have another manufacturer is absurd, plus what happens if the company gets bought out or renamed? Not withstanding that no-one really cares who the manufacturer of it is, if you have a board with the part, you are going to be looking up the part number, you're not going to look up "ADI" then go through every single drivers Kconfig to see if you can find one that works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the reasoning behind it and have updated the PR accordingly. 👍
Add/update guidelines on how KConfig files should be written and organized by identifying a common style currenlty used in most areas of the project and formalizing it. This should provide an aid for future merge request, code reviews and refactorings to enable the acting party to better enforce consistency in KConfig files. Signed-off-by: Tim Schrader <[email protected]>
cd4e944
to
f7bacd2
Compare
.. literalinclude:: kconfig_example_driver.txt | ||
:language: kconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of copying the file, just include the actual file
.. literalinclude:: kconfig_example_driver.txt | |
:language: kconfig | |
.. literalinclude:: ../../../drivers/adc/Kconfig.ad4114 | |
:language: kconfig |
Same for the sensor sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the examples are not a 1 to 1 copy of the drivers Kconfig (e.g. the AD4114 Kconfig does not use menuconfig
)
If we want to use real Kconfig files I would exchange them with the first refactorings in a different PR, as I first want to have an agreed style before changing the code.
Also why we might not want to remove the samples:
- I wanted to show a "simple" and a "complex" configuration. When using real drivers config, the simple example might get extended without the documentation getting updated with another simple example
- The document looses the code annotations to outline what an "enabling symbol" and "config symbol" is
|
@@ -0,0 +1,8 @@ | |||
config SENSOR_BMP581 | |||
bool "BMP581 barometric pressure sensor driver" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these (and the drivers) have the depends on
and select
and default
statements?
Add/update guidelines on how KConfig files should be written and organized by identifying a common style currenlty used in most areas of the project and formalizing it.
This should provide an aid for future merge request, code reviews and refactorings to enable the acting party to better enforce consistency in KConfig files.
Part of #7272