-
Notifications
You must be signed in to change notification settings - Fork 91
Conversation
94ca470
to
3da9115
Compare
@@ -57,23 +50,20 @@ Added a `{user}` role to Sphinx config -- by {user}`webknjaz` | |||
File `docs/changelog-fragments.d/116.feature.md`: | |||
|
|||
```md | |||
Added support for nested module options (suboptions) | |||
-- by {user}`tomaciazek` |
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 change doesn't make sense, it was demonstrating that it is okay to have more than one line in fragments.
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.
That is clearly an unbacked statement as there is no need need to wrap text to show people they can wrap it or not. We are not in the business to teach people markdown syntax.
If you really wanted to demonstrate it you would have included something like `This can also wrap on multiple lines." as part of the fragment itself.
As we would introduce automatic formatting there we have no need to tell user how and where to wrap the text as that will be done automatically.
|
||
## Adding change notes with your PRs | ||
|
||
It is very important to maintain a log for news of how |
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 huge rewrite contributes to the complexity of synchronizing this text across projects 👎
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.
@ansible/devtools team never agreed on something like this. We do not have a reference sync source and the reality is that even if we would do, we could implement the same formatting on the other one.
Basically, i see the comment as diverging from the scope of the PR and no real reasons for excluding this file, especially as it will render any attempt to touch it with vscode a nightmare as prettier will reformat it.
948db82
to
cf49403
Compare
Ensures that all our references keys are using lowercase in order to workaround known prettier bug. Related: prettier/prettier#3835 Required-By: ansible#177
Ensures that all our references keys are using lowercase in order to workaround known prettier bug. Related: prettier/prettier#3835 Required-By: ansible#177
cf49403
to
44eca4a
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.
I'm still against lowecasing. Please respect the previous agreement of not marking conversations as resolved because the requests there are not resolved.
44eca4a
to
11aaf0f
Compare
That floating comment makes no sense because there is no lowercase change as part of this PR. The lowercasing was removed from this PR and that is why the original comment was marked as resolved, as it was not longer applying to that change. |
It was here when I asked not to do it, and it was still implemented by sneakily merging it despite the request. If there is no value in giving reviews and people merge things via by separating patches and hiding them from reviewers who explicitly asked not to do them in the first place, is that our new accepted way of collaboration? Do you expect me to use this new best practice as well as a primary workaround? |
It was not resolved, you did exactly opposite to what was asked. Is this what resolved actually means now? |
This should ensure that we no longer get surprise changes when we edit tracked
.md
files in vscode as it applies all prettier rules, while still passing our markdown linting.Please note that markdown files touched by this change are result of running pre-commit, no other manual changes were made.
Based on ansible/vscode-ansible#386