-
Notifications
You must be signed in to change notification settings - Fork 292
refactor: coding style cleanup #1596
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
Conversation
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.
Hey @jrson83 I added a comment on the 3 left adr changes.
Regarding the indenting of 2 spaces. What tool did you use to correct or identify those. make fix
will not detect changes as far as I could tell from running it from your branch and reverting some of it, there is no info or fix 🤔
@@ -81,3 +81,4 @@ It would cross the design idea of unified titles though and should only be consi | |||
> *When creating notifications, just decide on the correct type of notification, | |||
add a meaningful message, don't waste even a thought on creating a title... | |||
And you're done!* | |||
|
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.
@jrson83 there are still 3 changes from the adrs left. Please remove them
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.
@jrson83 there are still 3 changes from the adrs left. Please remove them
I am currently unable to operate due to physical exhaustion. I will take care of it as soon as I am able to.
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.
@jrson83 get well soon 🛌 🙁
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.
@Isengo1989 Thanks, I feel better now.
So we might have a conflict with this last file:
resources/references/adr/2023-02-27-native-extension-system-with-vue.md
In the original file, there is a final new line, which may also include either a space or a tab character. I'm not sure which one if it is, I will try to insert a space
in the final new line and commit the file.
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.
OK that did not work at all. It seams there is a conflict with all the 3 files, cause of different indentation, I cannot revert because the indentation is now forced by the .editorconfig
😟
The inconsistency in indentation in the markdown files you see can happen due to several reasons, especially when there is no shared
A
There are two things to bear in mind during the changeover. Git tracks content changes rather than formatting differences like whitespace indentation when it doesn't affect the file's functionality or content meaning. Git can ignore changes in whitespace unless explicitly configured to track them. However, since our goal is to create a standardized mechanism with an indentation width of 2 in order to work with a consistent indentation size in every IDE and also with markdownlint, only the What I have done here is converted all tabs to spaces with the M010 rule: So I didn't actually fix the problem, but used the I reformatted the xml, html and php code blocks with a vscode extension (with https://marketplace.visualstudio.com/items?itemName=ame-neko.markdown-code-block-formatter This extension can use different configured formatter for any code-block language: PHP: XML: YAML: JS/TS: Oh and markdownlint does not care about valid indentation in code-blocks at all. |
@Isengo1989 With our discussion in mind, I think we can maybe cherry pick some commits: Valid, but de-aliased: See about how much conficts do we create with those: We could also |
Hey @jrson83 thx for elaborating. The issue we are having is that the indention is currently not reproducible in a workflow / check, as you mentioned this is not typically done by markdownlinters nor by the ide settings. Not sure how the IDE behaved if you copy code from another project and paste it in the code block 🤔 But that just said as a side note. I would agree with @bojanrajh for the time being to cherry pick some commits in order to get the new spacing ready to go in the IDEs and fix comments / code block definitions (PHP -> php ) and some others. We do have some bigger PRs coming in for 6.7 and a restructuring draft of a lot of content. If those are merged and we can take a closer look at fixing the existing indenting to be up to date. That lead to my question if there was a tool you used to fix those ( I suppose you used the plugins and did a "reformat" in vscode). We can also look at the tool @bojanrajh mentioned (not familiar with it yet). |
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.
Copilot reviewed 301 out of 311 changed files in this pull request and generated no suggestions.
Files not reviewed (10)
- .editorconfig: Language not supported
- .vscode/settings.json: Language not supported
- Makefile: Language not supported
- guides/hosting/infrastructure/message-queue.md: Evaluated as low risk
- guides/hosting/infrastructure/rate-limiter.md: Evaluated as low risk
- concepts/commerce/checkout-concept/cart.md: Evaluated as low risk
- guides/hosting/configurations/framework/samesite-protection.md: Evaluated as low risk
- guides/hosting/configurations/shopware/staging.md: Evaluated as low risk
- guides/hosting/infrastructure/filesystem.md: Evaluated as low risk
- guides/hosting/configurations/observability/profiling.md: Evaluated as low risk
@bojanrajh @Isengo1989 this sounds great to me. It would be great if the indentation could also be applied to the inported files, if this is possible with the workflow mentioned. |
Thanks guys, closing in favour of #1629 |
This PR fixes multiple issues I encountered when working on #1562. It includes the following changes, I tried to break into multiple commits:
editorconfig
make fix
runs still with successBy providing an
.editorconfig
phpstorm users should automatically make use of the specified settings. To provide (force) consistancy for vscode users without the need of using third party plugins, a minimal.vscode/settings.json
config is required.Since markdownlint is not intended to format language specific code in code blocks (e.g. javascript) it took longer then I thought. But in the end I think this PR will help to enforce a consistent coding/docs style.