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

KSType.toTypeName fixed to work with aliased types #1534

Merged
merged 8 commits into from
Apr 28, 2023
Merged

KSType.toTypeName fixed to work with aliased types #1534

merged 8 commits into from
Apr 28, 2023

Conversation

Squiry
Copy link
Contributor

@Squiry Squiry commented Apr 28, 2023

resolves #1513

ZacSweers
ZacSweers requested changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Please add a test that would have failed prior to this change and please write a useful description for the changes in the PR

...p/test-processor/src/main/kotlin/com/squareup/kotlinpoet/ksp/test/processor/TestProcessor.kt Outdated Show resolved Hide resolved
...p/test-processor/src/main/kotlin/com/squareup/kotlinpoet/ksp/test/processor/TestProcessor.kt Outdated
@@ -48,13 +41,28 @@ import com.squareup.kotlinpoet.ksp.writeTo
class TestProcessor(private val env: SymbolProcessorEnvironment) : SymbolProcessor {

private val unwrapTypeAliases = env.options["unwrapTypeAliases"]?.toBooleanStrictOrNull() ?: false
private val toTypeNameMode = ToTypeNameMode.valueOf(env.options["toTypeNameMode"] ?: "REFERENCE")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The choice is to add parameter or to copy paste all the code from TestProcessor and replace KSTypeReference.toTypeName calls with .resolve().toTypeName() and copy paste tests then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But why are you testing both paths? I'm looking for an explanation of what benefit this adds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why are you testing both paths? I'm looking for an explanation of what benefit this adds

We need to test both paths anyway, I can't write test case explicitly for my case without modifying existing processor (or writing a new one).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What tests fail with your test processor changes without your change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smokeTest and regression_1304_with_type_parameters

@Squiry
Copy link
Contributor Author

Squiry commented Apr 28, 2023

I added test case that shows problem separated.

Squiry and others added 4 commits April 28, 2023 19:17
@Squiry
import fix
2262800
@Squiry
imports fix
0b8ce98
@ZacSweers
Simplify test + fix formatting/style issues
d94ac98
@ZacSweers
Merge remote-tracking branch 'Squiry/kstype-totypename-fix' into ksty…
cd14f39 
…pe-totypename-fix

# Conflicts:
#	interop/ksp/test-processor/src/test/kotlin/com/squareup/kotlinpoet/ksp/test/processor/TestProcessorTest.kt
ZacSweers
ZacSweers approved these changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Cleaned it up a bit. Thanks!

@ZacSweers ZacSweers merged commit c7fe08f into square:master Apr 28, 2023
@Squiry Squiry deleted the kstype-totypename-fix branch April 28, 2023 17:09
github-merge-queue bot referenced this pull request in slackhq/circuit May 6, 2023
@slack-oss-bot @ZacSweers
Update kotlinpoet to v1.13.2 (#614)
d369858 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.squareup:kotlinpoet-ksp](https://togithub.com/square/kotlinpoet)
| dependencies | patch | `1.13.1` -> `1.13.2` |
| [com.squareup:kotlinpoet](https://togithub.com/square/kotlinpoet) |
dependencies | patch | `1.13.1` -> `1.13.2` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>square/kotlinpoet</summary>

###
[`v1.13.2`](https://togithub.com/square/kotlinpoet/releases/tag/1.13.2)

[Compare
Source](https://togithub.com/square/kotlinpoet/compare/1.13.1...1.13.2)

##### What's Changed

- KSType.toTypeName fixed to work with aliased types by
[@&#8203;Squiry](https://togithub.com/Squiry) in
[https://github.com/square/kotlinpoet/pull/1534](https://togithub.com/square/kotlinpoet/pull/1534)

##### New Contributors

- [@&#8203;Squiry](https://togithub.com/Squiry) made their first
contribution in
[https://github.com/square/kotlinpoet/pull/1534](https://togithub.com/square/kotlinpoet/pull/1534)

**Full Changelog**:
square/kotlinpoet@1.13.1...1.13.2

</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 these
updates 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:eyJjcmVhdGVkSW5WZXIiOiIzNS43MS42IiwidXBkYXRlZEluVmVyIjoiMzUuNzEuNiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: Zac Sweers <pandanomic@gmail.com>
ZacSweers referenced this pull request in ZacSweers/CatchUp May 6, 2023
@renovate
Update dependency com.squareup:kotlinpoet to v1.13.2 (#676)
d093605 
[![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.squareup:kotlinpoet](https://togithub.com/square/kotlinpoet) |
`1.13.1` -> `1.13.2` |
[![age](https://badges.renovateapi.com/packages/maven/com.squareup:kotlinpoet/1.13.2/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/maven/com.squareup:kotlinpoet/1.13.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/maven/com.squareup:kotlinpoet/1.13.2/compatibility-slim/1.13.1)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/maven/com.squareup:kotlinpoet/1.13.2/confidence-slim/1.13.1)](https://docs.renovatebot.com/merge-confidence/)
|

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>square/kotlinpoet</summary>

###
[`v1.13.2`](https://togithub.com/square/kotlinpoet/releases/tag/1.13.2)

[Compare
Source](https://togithub.com/square/kotlinpoet/compare/1.13.1...1.13.2)

#### What's Changed

- KSType.toTypeName fixed to work with aliased types by
[@&#8203;Squiry](https://togithub.com/Squiry) in
[https://github.com/square/kotlinpoet/pull/1534](https://togithub.com/square/kotlinpoet/pull/1534)

#### New Contributors

- [@&#8203;Squiry](https://togithub.com/Squiry) made their first
contribution in
[https://github.com/square/kotlinpoet/pull/1534](https://togithub.com/square/kotlinpoet/pull/1534)

**Full Changelog**:
square/kotlinpoet@1.13.1...1.13.2

</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://app.renovatebot.com/dashboard#github/ZacSweers/CatchUp).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS43MS40IiwidXBkYXRlZEluVmVyIjoiMzUuNzEuNCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacSweers ZacSweers ZacSweers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KSType.toTypeName function returns ClassName for alias type instead of ParameterizedTypeName
2 participants
@Squiry @ZacSweers