IEP-1445 GH #1148: Avoid creating '.clang-format' on each compilation #1150
Conversation
WalkthroughThe pull request introduces enhancements for automating the creation of the Clang format file. It adds new constants in the core preferences to manage this automation. A new method in the Clang format handler encapsulates the logic for determining file creation based on file existence and user preferences. Additionally, a new button is added to the preferences page, allowing users to enable or disable this automation feature. Localization entries and tooltips are also provided to support the new user options. Changes
Sequence Diagram(s)sequenceDiagram
participant CFH as ClangFormatFileHandler
participant PS as Platform/Preference Store
participant FS as File System
CFH->>CFH: update()
CFH->>CFH: shouldCreateClangFormat()
CFH->>PS: Retrieve AUTOMATE_CLANGD_FORMAT_FILE preference
PS-->>CFH: Return boolean value
alt File missing and automation enabled
CFH->>FS: Create .clang-format file
else
CFH-->>CFH: Skip file creation
end
sequenceDiagram
participant U as User
participant EP as EspresssifPreferencesPage
participant PS as Preference Store
U->>EP: Toggle automateClangdFormatCreationBtn
EP->>PS: Save updated preference (AUTOMATE_CLANGD_FORMAT_FILE)
PS-->>EP: Preference updated
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ClangFormatFileHandler.java (2)
28-40: Update JavaDoc to reflect the configurable behavior.The method's behavior has changed to respect user preferences, but this isn't reflected in the documentation.
Apply this diff to update the JavaDoc:
/** - * Updates the .clang-format file. If the file does not exist, it is created and initialized with default settings. + * Updates the .clang-format file. If the file does not exist and automatic creation is enabled in preferences, + * it is created and initialized with default settings. * * @throws IOException if an I/O error occurs during file creation or writing * @throws CoreException if an error occurs while refreshing the project */
34-45: Consider validating existing .clang-format files.The current implementation only checks for file existence, but not its validity. Consider adding validation for existing
.clang-formatfiles to ensure they are well-formed.This would help catch corrupted files early and prevent potential issues with clang formatting. I can help implement the validation logic if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ClangFormatFileHandler.java(3 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java(7 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2)
30-31: LGTM! Well-structured preference constants.The new constants follow the established naming patterns and properly use the NON-NLS tag. Setting the default to
truemaintains backward compatibility with existing behavior.
45-45: LGTM! Consistent formatting.The added empty line maintains consistent spacing between different sections of constants.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ClangFormatFileHandler.java (2)
13-16: LGTM!The new imports are necessary for accessing platform preferences and core constants to support the configurable
.clang-formatfile creation feature.
47-52: LGTM!The method implementation is clean and follows best practices:
- Clear and descriptive method name
- Correct logic combining file existence check with user preference
- Proper use of preference service API
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1)
8-9: LGTM! New localization strings follow existing patterns.The new string constants for the Clang format automation button are properly declared and follow the established naming conventions.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (2)
175-180: LGTM! Clean implementation of the new preference option.The new checkbox for controlling Clang format automation is well-implemented:
- Properly placed in the build settings group
- Follows the established pattern for preference options
- Uses localized strings for text and tooltip
268-269: LGTM! Proper error handling for preference updates.The new preference value is saved within the existing try-catch block, ensuring errors are properly logged and handled.
...les/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
Outdated
Show resolved
Hide resolved
| EspresssifPreferencesPage_AutoCreateClangFormatBtnName=Auto-create .clang-format | ||
| EspresssifPreferencesPage_AutoCreateClangFormatTooltipMsg=Ensures .clang-format exists, generating it if missing after project creation and on each build. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix spelling and improve message organization.
- The key prefix has a spelling error: "Espresssif" should be "Espressif" (3 's' vs 2 's').
- Consider moving these entries near other build-related settings for better organization.
- The tooltip message could be more concise.
Apply this diff to fix the issues:
-EspresssifPreferencesPage_AutoCreateClangFormatBtnName=Auto-create .clang-format
-EspresssifPreferencesPage_AutoCreateClangFormatTooltipMsg=Ensures .clang-format exists, generating it if missing after project creation and on each build.
+EspressifPreferencesPage_AutoCreateClangFormatBtnName=Auto-create .clang-format
+EspressifPreferencesPage_AutoCreateClangFormatTooltipMsg=Automatically generate .clang-format file if missing during project creation and builds.Move these entries after line 3 (EspresssifPreferencesPage_BuildGroupTxt) to group them with other build-related settings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EspresssifPreferencesPage_AutoCreateClangFormatBtnName=Auto-create .clang-format | |
| EspresssifPreferencesPage_AutoCreateClangFormatTooltipMsg=Ensures .clang-format exists, generating it if missing after project creation and on each build. | |
| # ... (other entries before line 3 remain unchanged) | |
| EspresssifPreferencesPage_BuildGroupTxt=Build Settings | |
| EspressifPreferencesPage_AutoCreateClangFormatBtnName=Auto-create .clang-format | |
| EspressifPreferencesPage_AutoCreateClangFormatTooltipMsg=Automatically generate .clang-format file if missing during project creation and builds. | |
| # ... (any entries after remain unchanged) |
…ces/EspresssifPreferencesPage.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@sigmaaa hi ! Tested under: Created/built the project with this option turned off and on, and verified that the Created LGTM 👍 |
|
@kolipakakondal @alirana01 hi ! Please, review |
There was a problem hiding this comment.
LGTM just need to confirm with @AndriiFilippov and @kolipakakondal if this is okay in terms of UX
Because I am not sure if this should be under Build settings or a separate editor settings.
There was a problem hiding this comment.
Can you add this to the Clangd editor documentation as well? Please explain the reason for doing this and mention that it is useful for projects migrated from earlier versions (2.x to 3.x) to ensure compatibility with the Clangd editor. Otherwise, it looks good to me.
Agree. We are doing this during the build to address old project migrations (2.x to 3.x). We could categorize this under "Clangd Settings" so that, going forward, any additional related settings can be added here. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java(7 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (4)
46-46: LGTM!The field declaration follows the class's naming convention and encapsulation principles.
89-101: LGTM!The method follows the established pattern for adding preference groups and correctly initializes the UI elements. The implementation aligns with the PR objective to add a user-friendly toggle for
.clang-formatfile creation.
276-277: LGTM!The preference saving logic is consistent with other similar settings and properly handles errors.
324-325: LGTM!The default value initialization is consistent with other similar settings.
| automateClangdFormatCreationBtn | ||
| .setSelection(getPreferenceStore().getBoolean(IDFCorePreferenceConstants.AUTOMATE_CLANGD_FORMAT_FILE)); |
There was a problem hiding this comment.
Use getDefaultBoolean for resetting to default value.
When resetting to defaults, use getDefaultBoolean instead of getBoolean to ensure the correct default value is used.
Apply this diff to fix the issue:
- .setSelection(getPreferenceStore().getBoolean(IDFCorePreferenceConstants.AUTOMATE_CLANGD_FORMAT_FILE));
+ .setSelection(getPreferenceStore().getDefaultBoolean(IDFCorePreferenceConstants.AUTOMATE_CLANGD_FORMAT_FILE));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| automateClangdFormatCreationBtn | |
| .setSelection(getPreferenceStore().getBoolean(IDFCorePreferenceConstants.AUTOMATE_CLANGD_FORMAT_FILE)); | |
| automateClangdFormatCreationBtn | |
| .setSelection(getPreferenceStore().getDefaultBoolean(IDFCorePreferenceConstants.AUTOMATE_CLANGD_FORMAT_FILE)); |
Hi @kolipakakondal, updated the documentation and updated Espressif preference page as follows: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs_readme/clangd_cdt_support.md (2)
54-56: Minor Whitespace AdjustmentsThe insertion of these blank lines (or marker lines) helps visually separate the sections, improving readability. There is no negative impact on the output.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
54-54: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
57-63: New Section: Automatic .clang-format CreationThis section clearly documents the new feature that automatically creates a
.clang-formatfile for each project. The step-by-step instructions (navigating to Window > Preferences > Espressif, locating theAuto-create .clang-formatoption, and applying changes) are concise and consistent with the overall style of the document.Suggestions:
- Verify that the option name (
Auto-create .clang-format) exactly matches what appears in the Preferences dialog.- Consider adding a screenshot or an additional note for users migrating from older plugin versions to further enhance clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs_readme/clangd_cdt_support.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs_readme/clangd_cdt_support.md
54-54: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
|
LGTM |
Description
Added an option to disable/enable auto-creation of clang-format file
Fixes # (IEP-1445)
Type of change
Please delete options that are not relevant.
How has this been tested?
Created/built the project with this option turned off and on, then checked if clang-format was created accordingly
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
.clang-formatfile..clang-formatfiles and how to manage this setting.