-
Notifications
You must be signed in to change notification settings - Fork 503
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
0.45.0
backwards incompatibility: KtLint.Params#userData
gets ignored, unclear migration path + more feedback
#1434
Comments
Unfortunately UserData is set in the data class
In a rule, the userData can be retrieved from the node:
The userData which is added to the
Data which is retrieved via As the userData is defined as In future releases, the Properties from
Breaking the API was definitively not intended. I have tried to deprecate while keeping support for old behavior but it seems that I missed a use case. I am affraid that our test coverage for API usage is insufficient. It would help, if you could share some unit tests that demonstratie what currently fail for you as API user. The recommendation for not skipping upgrades is that the deprecation periods will be shortened to maybe as short as 1 release. I would like to evolve the API fast instead of having to spend a lot of effort in keeping backwards compatibility over a series of releases.
Perhaps, you have missed section "Retrieving ".editorconfig" property value" in the release notes.
Ow, that could lead to problem when rules are using the Previously this lint method using the
I hope that I have properly answered your questions. If not, feel free to ask more questions. But it might be a better idea to continue in separate issues... |
This change has made KtLint unusable in essentially every third party plugin for it (gradle plugins, etc) because they all relied on userData. While I understand it's complicated, I think it should be temporarily restored until there's a proper path forward for integrations |
I don't think restoring is the way forward. We need to figure out how we can move forward. But this can only be done with input from the community. So lets work together to identify problems like @mateuszkwiecinski did and define possible solutions for it. Those solutions should then be release as 0.45.x till we have a working release for the API consumers again. So I welcome proposals to the different problems mentioned above. |
Most problems seem not that difficult to fix. I am working on it and expect it to fix next week. |
tbh, given that ktlint is still not at 1.0.0, I think it's probably fine to just break API and document the migration path properly. I'd pretty much prefer a compile/runtime exception rather than this hidden behavior of |
Thanks, Paul, for explaining what the
Here's something that passes on @Test
fun repro() {
val lintErrors = mutableListOf<LintError>()
KtLint.lint(
KtLint.Params(
userData = mapOf("indent_size" to "2"),
text = """
class WrongFileName {
fun someFun() = 2
}
""".trimIndent(),
cb = { lintError, _ ->
lintErrors.add(lintError)
},
ruleSets = listOf(StandardRuleSetProvider()).map(StandardRuleSetProvider::get)
)
)
check(lintErrors.isEmpty())
} hope that's helpful :)
I'm only the API consumer (I hope we agree the public methods of
I have one more question, about my third point, I'm not sure if I understood what should I/we do to properly use ktlint. You mentioned
Does it mean basically all integrations use ktlint in a wrong, unexpected way? 🤔 |
In this test the userData does contains the The test above has to be rewritten as follows:
"Retrieving" an I agree with you that public methods in general should be considered the public API. Currently Ktlint exposes public methods which we want to remove from the public API. We want to offer a clean, small and well documented API. We have some iterations to go before we will arrive at that point. Indeed, I am not sure how
Tnx again for your questions and remarks. |
…45.1. * Deprecate data class 'Params' as it only provides the userData parameter which prohibits API Consumers to split the '.editorconfig' properties from the other user data. * Deprecate function 'toExperimentalParams' to force API Consumers to start using the 'ExperimentalParams'. * In ExperimentalParams clarify that 'userData' should not contain any '.editorconfig' properties. Now also a runtime error is thrown whenever the lint/format methods are called and 'userData' contains '.editorconfig' properties. * Remove confusing TODO's about moving methods 'lint' and 'format' to the 'ktlint-test' module. * Update deprecation messages to clarify intention. * The 'lint' and 'format' methods in module 'ktlint-core' no longer call the 'VisitorProvider' with parameter 'isUnitTestContext' enabled. API Consumers call these methods with production code for which this parameter should be disabled. * The RuleExtension calls the 'lint' and 'format' methods but do provide a VisitorProvider for which the parameter 'isUnitTestContext' is enabled. * Deprecate the 'format' which accepted the 'ExperimentalParams' and 'Iterable<RuleSet>' while the latter is already included in the first. * Add new 'format' method which accepts 'ExperimentalParams' and 'VisitorProvider' only. * Move 'EditorConfigOverride' from 'ktlint-test' to 'ktlint-core' Closes pinterest#1434
Thanks for a detailed response 🙏
I can only describe my use-case, I won't represent all other applications. So, kotlinter-gradle (which I occasionally contribute to) is a Gradle wrapper around
plugin users don't interact with
Would be: to verify if Gradle plugin consumer can still override Now, seeing I want to raise one more topic. |
Thank you for all your work @paul-dingemans and recognize you're shepherding inherited code with some wrinkles. For kotlinter-gradle at least, we're quite fine having a compatibility boundary where kotlinter-gradle version, say 4.0.0, can only be used with ktlint 0.45.2 or later. Also quite fine removing some of the params like With this in mind, could we move forward with more confidence as @mateuszkwiecinski implies -- Generally a gradle wrapper's needs in a ktlint API are:
|
Tnx!
In next release (0.46.0), I will definitely clean up a lot of the alpha and beta markings. I am sure that more changes will follow in the future but both maintainers and consumers now need an API which is less bloated. Hopefully this also sheds light on the next steps. Having said that, I expect following to be included in the release:
Please file a separate issue. But please make more clear what you need. It can take a while before it will be implemented. My own backlog is also full of stuff that I want to achieve. And as winter is almost gone, I will have less time to spend. |
…45.1 (#1442) * Fix backward incompatibility issues released via KtLint 0.45.0 and 0.45.1. * Deprecate data class 'Params' as it only provides the userData parameter which prohibits API Consumers to split the '.editorconfig' properties from the other user data. * Deprecate function 'toExperimentalParams' to force API Consumers to start using the 'ExperimentalParams'. * In ExperimentalParams clarify that 'userData' should not contain any '.editorconfig' properties. Now also a runtime error is thrown whenever the lint/format methods are called and 'userData' contains '.editorconfig' properties. * Remove confusing TODO's about moving methods 'lint' and 'format' to the 'ktlint-test' module. * Update deprecation messages to clarify intention. * The 'lint' and 'format' methods in module 'ktlint-core' no longer call the 'VisitorProvider' with parameter 'isUnitTestContext' enabled. API Consumers call these methods with production code for which this parameter should be disabled. * The RuleExtension calls the 'lint' and 'format' methods but do provide a VisitorProvider for which the parameter 'isUnitTestContext' is enabled. * Deprecate the 'format' which accepted the 'ExperimentalParams' and 'Iterable<RuleSet>' while the latter is already included in the first. * Add new 'format' method which accepts 'ExperimentalParams' and 'VisitorProvider' only. * Move 'EditorConfigOverride' from 'ktlint-test' to 'ktlint-core' Closes #1434
@mateuszkwiecinski @jeremymailen Changes have been merged to master. It would be great if you could check out against the snapshot whether the compatibility issues have been resolved. If so, then I will request the patch release to be build. |
I tested the latest snapshot against So if I was asked if |
Yes, thanks I think we're good to move ahead. |
Thnx! @shashachu Would you be so kind to release current master as version 0.45.2? |
0.45.2 is released. Thanks everyone! |
Initially reported on Kotlin slack: https://kotlinlang.slack.com/archives/CKS3XG0LS/p1647897839331439 but I was asked to create issue here.
Expected Behavior
0.45.0
is backward compatible with0.44.0
, or has clear migration pathObserved Behavior
I'll copy-paste what I wrote on Slack:
userData
map, does it only change its purpose, or will it be removed completely at some point? The code I was upgrading had been passingindent_size
anddisabled_rules
withinuserData
map. The migration path forindent_size
seemed to be hidden but I was able to identifyDefaultEditorConfigProperties.indentSizeProperty
which looked legitimate and worked in my case, but I couldn’t find a replacement fordisabled_rules
. The factExperimentalParams
still takesuserData
suggests that’s still the “recommended” option, am I wrong here? What’s the proper way to pass rules that I expect to be disabled?0.45.0
, where values passed asuserData
using the now deprecatedlint
method are ignored. Maybe I misread the release notes, but they had vibe you strongly advise to migrate now, because later it will be more difficult, but in overall0.45.0
is still backwards compatible. If breaking the compatibility was intended, then I’ll share my personal opinion: I think it would be better for api consumers if you removed the method already in this release so api consumers would get a compile-time error, instead of runtime one (or bug reports from end users 😛). If breaking the compatibility wasn’t intended, then FYI it did, at least the case described above 😛And just for context: I attempted to bump ktlint in Gradle Plugin, so I might be missing something here, as the release notes weren’t 100% clear for me (for example: I’ve never seen
ASTNode.getEditorConfigValue(editorConfigProperty)
orEDITOR_CONFIG_USER_DATA_KEY
mentioned there 😬 )isUnitTestContext = true
gets passed when calling any ofKtLint#lint
methods. It doesn’t break anything, but looks suspicious, so I thought it might be worth pointing out, especially when seeing todos like this 👇AFAIU both ktlint-gradle and kotlinter-gradle use lint and format in regular usage, not in tests soo… 👀
Feel free to split them into 3 separate issues if it's more convenient for you :)
Steps to Reproduce
Run
KtLint.lint(KtLint.Params(userData = mapOf("indent_size" to "5"), ...))
on a file with respective indentation and observe the lint reporting failures.Your Environment
.editorconfig
settings: noneExperimentalParams
class which introduced even more confusion - hence all the questions abovemacos-latest
,windows-latest
The text was updated successfully, but these errors were encountered: