-
Notifications
You must be signed in to change notification settings - Fork 81
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
@sample tags should not be moved within kdoc #406
Comments
Thank you @liutikas for the thorough report! If I remember right, this is intentional, and all tags appear at the end of rendered kdocs. They're also generally multiline, so if you have a block like this:
You might be surprised to find that That said, let me double check and see if cc @tnorbye |
Can you explain why you find it surprising that
I'll discuss this with jetbrains offline next week during google/jetbrains sync to clarify what they intend specs to be. In general, if you don't support this in the tool, we'll likely have to fork ktfmt which i'd really like to avoid. |
Sure, what I mean is that it is included in the sample, kinda. You'll find that with that setup, C is not in its own paragraph. It also doesn't appear in sample, but that's just because everything after the first ref is discarded (as far as I can tell). Let me know what you find out from jetbrains. And like I mentioned, if you can share a sample of how a kdoc renderer would actually process your example, it'll be easier to evaluate what's best to do. |
@liutikas here's a better example, which demonstrates what I mean. You'll notice in the rendered kdoc that the paragraph between the samples is lost (the line that reads "But you can go even higher!") Also, here's how Android Studio formats the example you gave in the post: |
The Kotlin plugin in the IDE does a really poor job rendering KDocs; it doesn't actually run dokka, it just has a light simulation, so it often gets things wrong (remember how we had the same issue a while back with hanging indents as well). What I usually do is to run Dokka to see how the documentation is rendered. I have a checkout of the Dokka github project, then I go into And this confirms that Dokka does move all embedded samples to a single section at the end. For example, given this patch:
|
Thanks for confirming @tnorbye. This also gives me two ideas:
|
TL;DR from the discussion between google and jetbrains:
Would ktfmt be willing to take a PR to add orderDocTags as an option to the formatter so we could adopt ktfmt without forking/ugly hacks? |
Thanks for the update @liutikas! Great to hear from jetbrains about it too.
I think @cgrushko can probably speak to this better than I can, but my understanding is that we want to keep options to an absolute minimum. If the spec was updated (or committed to be updated) to support inline sample tags, then it seems like a no-brainer that we'd implement that. But since you're asking for an option, that tells me you'd want this ability even if it doesn't make it into the spec. Am I understanding right? My only question there is - what scenario would be supporting exactly? If none of the existing tools support rendering inline sample tags, what use would it be for ktfmt to support it? That said, I think there's some precedent from #17, where @strulovich added a flag to disable optimizing imports. Basically, we could take the stance that Thoughts? |
I was curious if yall are willing to add an option to disable tag reordering? We finally migrated to ktfmt and @sample reordering is causing us issues. |
I think my question from last time still stands:
One thing I can suggest is ditching cc @hick209 |
In androidx, we use a dokka-devsite-plugin built on top of dokka. This tool is used to generate d.android.com documentation for all of androidx code. dokka-devsite-plugin is also used by Android Gradle Plugin, Google Play Service and few other projects to generate their public facing user documentation. As androidx.compose developed, we found that supporting inline and more than one Since, we have also added support to Android Studio to render Have we made a mistake forking kdoc that had a vague spec for this? Possibly! Sadly, we have a huge codebase now, with close to a thousand |
I created #474 to disable ordering the doc tags, following a similar route to that in disabling unused imports. This is how our Kdoc would look like: The only diff it makes is removing the space between the last Reading the discussion above, this seems like the path of least resistance? |
@omarismail94 thanks for contributing! That said, right now my thinking is to go the route of special-casing @liutikas I want to check two things:
Once we have the code / results to compare we can make the final decision, but I think there's enough utility here to warrant the special case. cc @hick209 |
Yes, it would work and in fact be a more preferred solution.
Can you ballpark estimate how large of a task would this be? |
Tested: Added unit test Also compiled the JAR and tested it running: ``` java -jar ./ktfmt-0.50-SNAPSHOT-jar-with-dependencies.jar --kotlinlang-style --do-not-order-doc-tags compose/runtime/runtime-saveable/src/commonMain/kotlin/androidx/compose/runtime/saveable/RememberSaveable.kt ``` and it did not re-order the kdcoc tags in the kotlin file, as expected Fixed: Issue facebook#406
I have an alternate PR, #475, to treat sample tags as a paragraph. Let me know what your thoughts are! These are the changes that would be made to our AndroidX codebase as a result: https://android-review.git.corp.google.com/c/platform/frameworks/support/+/3117811 , (they don't affect rendering) |
Thanks @omarismail94! I actually just implemented this in 2ba3b09, in which I classify Priority tags appear before other tags, but only if they're before them prior to formatting. @liutikas I hope this will work for your use case. Let me know if you can do any validation on your side (I at least checked the example you provided) and we can work on releasing a new version. |
@davidtorosyan I just pulled your change and ran locally. Besides the tests failing, the change still isn't working for us. See screenshot below of diff: The change causes a lot of movement in our codebase, see here for diff: |
@omarismail94 I just fixed the tests with: babb277
I thought there might be cases like this. I'm not sure if that particular example makes sense to preserve though. Please note that The case in the OP on the other hand has clear utility.
I don't have access to that link. In general I don't think movement is bad thing, you'd expect that when turning on a formatter. I'm more worried about if the end result is worse or not. Could you provide some more screenshots maybe? |
@davidtorosyan I think the change works ! Below are examples of the type of diffs Ive seen. If the line seperator can't be re-added, when do you think you'd be able to release a new version with this change?
Our intended function was probably to have |
Can we add:
In ParagraphListBuilder to get the new line back? |
@omarismail94 if we do this:
It should be by inserting a newline between priority tags and other tags. i.e. That said I don't think special casing that is worth it. It shouldn't affect rendering, and IMO would be a controversial style change. Our policy is to avoid changing the formatting for style preference (e.g. placing a newline between tags or not).
I think we can work on this now, will update soon. |
Great, thanks!! |
Just created this release here https://github.com/facebook/ktfmt/releases/tag/v0.50 Closing issue, but please let us know if you are still facing issues |
Thanks @hick209 and @davidtorosyan for pushing this through! JAR works great! Hope to see it as part of the IntelliJ plugin soon |
Good call out @omarismail94, let me check why it was not published alongside the release |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.facebook:ktfmt](https://togithub.com/facebook/ktfmt) | dependencies | minor | `0.49` -> `0.50` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>facebook/ktfmt (com.facebook:ktfmt)</summary> ### [`v0.50`](https://togithub.com/facebook/ktfmt/releases/tag/v0.50): 0.50 #### Changelog - Add pre commit hooks to readme ([https://github.com/facebook/ktfmt/pull/462](https://togithub.com/facebook/ktfmt/pull/462)) – [@​0x26res](https://togithub.com/0x26res) - Add homebrew installation note to readme ([https://github.com/facebook/ktfmt/pull/468](https://togithub.com/facebook/ktfmt/pull/468)) – [@​chenrui333](https://togithub.com/chenrui333) - Refactor CLI argument parsing ([https://github.com/facebook/ktfmt/pull/467](https://togithub.com/facebook/ktfmt/pull/467)) – [@​grodin](https://togithub.com/grodin) - Fix issue with context receive in lambdas ([https://github.com/facebook/ktfmt/issues/471](https://togithub.com/facebook/ktfmt/issues/471)) – [@​hick209](https://togithub.com/hick209) - Don't reorder [@​sample](https://togithub.com/sample) tag ([https://github.com/facebook/ktfmt/issues/406](https://togithub.com/facebook/ktfmt/issues/406)) – [@​davidtorosyan](https://togithub.com/davidtorosyan) **Full Changelog**: facebook/ktfmt@v0.49...v0.50 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTYuMSIsInVwZGF0ZWRJblZlciI6IjM3LjM5Ni4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.facebook:ktfmt](https://togithub.com/facebookincubator/ktfmt) | dependencies | minor | `0.49` -> `0.50` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>facebookincubator/ktfmt (com.facebook:ktfmt)</summary> ### [`v0.50`](https://togithub.com/facebook/ktfmt/releases/tag/v0.50): 0.50 #### Changelog - Add pre commit hooks to readme ([https://github.com/facebook/ktfmt/pull/462](https://togithub.com/facebook/ktfmt/pull/462)) – [@​0x26res](https://togithub.com/0x26res) - Add homebrew installation note to readme ([https://github.com/facebook/ktfmt/pull/468](https://togithub.com/facebook/ktfmt/pull/468)) – [@​chenrui333](https://togithub.com/chenrui333) - Refactor CLI argument parsing ([https://github.com/facebook/ktfmt/pull/467](https://togithub.com/facebook/ktfmt/pull/467)) – [@​grodin](https://togithub.com/grodin) - Fix issue with context receive in lambdas ([https://github.com/facebook/ktfmt/issues/471](https://togithub.com/facebook/ktfmt/issues/471)) – [@​hick209](https://togithub.com/hick209) - Don't reorder [@​sample](https://togithub.com/sample) tag ([https://github.com/facebook/ktfmt/issues/406](https://togithub.com/facebook/ktfmt/issues/406)) – [@​davidtorosyan](https://togithub.com/davidtorosyan) **Full Changelog**: facebook/ktfmt@v0.49...v0.50 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTYuMSIsInVwZGF0ZWRJblZlciI6IjM3LjM5Ni4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.facebook:ktfmt](https://togithub.com/facebook/ktfmt) | `0.49` -> `0.51` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.facebook:ktfmt/0.51?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.facebook:ktfmt/0.51?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.facebook:ktfmt/0.49/0.51?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.facebook:ktfmt/0.49/0.51?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>facebook/ktfmt (com.facebook:ktfmt)</summary> ### [`v0.51`](https://togithub.com/facebook/ktfmt/blob/HEAD/CHANGELOG.md#051) ##### Added - Created CHANGELOG.md - Added --help option to CLI ([https://github.com/facebook/ktfmt/pull/477](https://togithub.com/facebook/ktfmt/pull/477)) ##### Changed - Preserves blank spaces between when clauses ([https://github.com/facebook/ktfmt/issues/342](https://togithub.com/facebook/ktfmt/issues/342)) - Named the default style as `Formatter.META_FORMAT` / `--meta-style` - `FormattingOptions` constructor parameters order was changed ##### Fixed - Compilation issues with online formatter (facebook/ktfmt@8605080) - Removing valid semicolons ([https://github.com/facebook/ktfmt/issues/459](https://togithub.com/facebook/ktfmt/issues/459)) - Incorrect detection of unused `assign` import ([https://github.com/facebook/ktfmt/issues/411](https://togithub.com/facebook/ktfmt/issues/411)) ##### Removed - **Deleted `Formatter.DROPBOX_FORMAT` / `--dropbox-style` (BREAKING CHANGE)** - Deleted `FormattingOptions.Style` enum ### [`v0.50`](https://togithub.com/facebook/ktfmt/releases/tag/v0.50): 0.50 #### Changelog - Add pre commit hooks to readme ([https://github.com/facebook/ktfmt/pull/462](https://togithub.com/facebook/ktfmt/pull/462)) – [@​0x26res](https://togithub.com/0x26res) - Add homebrew installation note to readme ([https://github.com/facebook/ktfmt/pull/468](https://togithub.com/facebook/ktfmt/pull/468)) – [@​chenrui333](https://togithub.com/chenrui333) - Refactor CLI argument parsing ([https://github.com/facebook/ktfmt/pull/467](https://togithub.com/facebook/ktfmt/pull/467)) – [@​grodin](https://togithub.com/grodin) - Fix issue with context receive in lambdas ([https://github.com/facebook/ktfmt/issues/471](https://togithub.com/facebook/ktfmt/issues/471)) – [@​hick209](https://togithub.com/hick209) - Don't reorder [@​sample](https://togithub.com/sample) tag ([https://github.com/facebook/ktfmt/issues/406](https://togithub.com/facebook/ktfmt/issues/406)) – [@​davidtorosyan](https://togithub.com/davidtorosyan) **Full Changelog**: facebook/ktfmt@v0.49...v0.50 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ZacSweers/CatchUp). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Repro steps
Expected
No changes are made
Actual
ktfmt format command will move
@param
tags to be next to@sample
with@sample
at the end.resulting in
Details
@sample
tags are sensitive to their location within kdoc as that indicates that location where the documentation tool should insert the referenced code.Using ktfmt 0.44
The text was updated successfully, but these errors were encountered: