Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add test examples for non-ideal situations #488

Closed
wants to merge 6 commits into from
Closed

Add test examples for non-ideal situations #488

wants to merge 6 commits into from

Conversation

snowe2010
Copy link
Contributor

No description provided.

@facebook-github-bot
Copy link
Contributor

Hi @snowe2010!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 19, 2024
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@snowe2010 snowe2010 mentioned this pull request Jun 19, 2024
Open
@facebook-github-bot
Copy link
Contributor

@davidtorosyan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

davidtorosyan
davidtorosyan requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@davidtorosyan davidtorosyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding more tests! I left some comments about moving them to the right places / adjusting the options. This is mainly to help with maintainability (the more tests have the same set of options, the easier to reason about).

core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated Show resolved Hide resolved
core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated Show resolved Hide resolved
core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated
)

@Test
fun `complex calls and calculation in named parameters with wrapping`() =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to FormatterTest.kt, omit formattingOptions if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so omitting the formatting options here showed another weird bug between google/kotlin formats. I moved it to FormatterTest and added another test to show the difference.

core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated Show resolved Hide resolved
core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Show resolved Hide resolved
snowe2010
snowe2010 commented Jun 27, 2024
View reviewed changes
Copy link
Contributor Author

@snowe2010 snowe2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidtorosyan I also added one more test around if expression functions

core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated
)

@Test
fun `complex calls and calculation in named parameters with wrapping`() =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so omitting the formatting options here showed another weird bug between google/kotlin formats. I moved it to FormatterTest and added another test to show the difference.

core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Outdated Show resolved Hide resolved
core/src/test/java/com/facebook/ktfmt/format/GoogleStyleFormatterKtTest.kt Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hick209
Copy link
Contributor

hick209 commented Aug 7, 2024

We apologize for the delay. Let's try to get another look into this

@facebook-github-bot
Copy link
Contributor

@hick209 merged this pull request in 937d21d.

github-merge-queue bot referenced this pull request in slackhq/foundry Aug 28, 2024
@slack-oss-bot @ZacSweers
Update dependency com.facebook:ktfmt to v0.52 (#926)
580dcf7 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.facebook:ktfmt](https://togithub.com/facebookincubator/ktfmt) |
dependencies | minor | `0.51` -> `0.52` |

---

> [!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.52`](https://togithub.com/facebookincubator/ktfmt/blob/HEAD/CHANGELOG.md#052)

##### Fixed

- IntelliJ plugin crash
([https://github.com/facebook/ktfmt/pull/501](https://togithub.com/facebook/ktfmt/pull/501))
- Ordering of `@property` and `@param` in KDoc
([https://github.com/facebook/ktfmt/pull/498](https://togithub.com/facebook/ktfmt/pull/498))
- Annotation in return expressions
([https://github.com/facebook/ktfmt/issues/497](https://togithub.com/facebook/ktfmt/issues/497))

##### Changed

- KotlinLang style also managing trailing commas
([https://github.com/facebook/ktfmt/issues/216](https://togithub.com/facebook/ktfmt/issues/216),
[https://github.com/facebook/ktfmt/issues/442](https://togithub.com/facebook/ktfmt/issues/442))
- Converted IntelliJ plugin to Kotlin
([https://github.com/facebook/ktfmt/pull/502](https://togithub.com/facebook/ktfmt/pull/502))

##### Added

- More stability tests
([https://github.com/facebook/ktfmt/pull/488](https://togithub.com/facebook/ktfmt/pull/488))
- Custom profile in plugin settings, mirroring Gradle/Maven plugins
([https://github.com/facebook/ktfmt/pull/503](https://togithub.com/facebook/ktfmt/pull/503))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, 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:eyJjcmVhdGVkSW5WZXIiOiIzOC40NS4wIiwidXBkYXRlZEluVmVyIjoiMzguNTIuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

---------

Co-authored-by: Zac Sweers <[email protected]>
@snowe2010 snowe2010 deleted the add-named-param-test-cases branch November 18, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtorosyan davidtorosyan davidtorosyan requested changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snowe2010 @facebook-github-bot @hick209 @davidtorosyan