-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: Add support for Checkstyle Configuration file properties #700
base: main
Are you sure you want to change the base?
feat: Add support for Checkstyle Configuration file properties #700
Conversation
@timtebeek I have created a blank test class. Can you please tell how I should test for the working of the checkstyle config file variables? |
Hi @SaptarshiSarkar12 ! You'll want to create matching files in https://github.com/openrewrite/rewrite-maven-plugin/tree/main/src/test/resources-its/org/openrewrite/maven as you can see there for other unit tests too. All those tests use https://khmarbaise.github.io/maven-it-extension/itf-documentation/usersguide/usersguide.html, which might help to read up on as well. Thanks for helping out here! |
Hi @timtebeek 👋! |
@timtebeek As a side question, is there a general rule of this project to assign the PRs to the contributors? I have seen projects assign the issues to the contributors. If this is a silly question, please do not mind 🙃. I asked this out of curiosity 😄. |
We typically assign the issue or pull request to whoever is working on it, also if they are community contributors. That way it's easy to see at a glance who (if anyone) is working on something from our project board. Sometimes when folks then later indicate they do not have the time or expertise to finish something, we can unassign them to indicate someone else needs to step up. Not a silly question at all; hope my answer helps you understand! |
Yeah, I understood. Thank you for the explanation @timtebeek 😁! |
@timtebeek Which recipe should I use in the plugin configuration for tests - the custom configuration that I made in strimzi/strimzi-kafka-operator#9422 or the one described in the docs of open-rewrite? |
|
Yeah, it should 😄. |
@timtebeek I have added a small test project in the resource-its folder as well as I have added a method in the test directory. Can you please check if those changes are right? Can you please tell me what are the next steps to do? |
@timtebeek Have you checked? |
@timtebeek Please tell me what changes are required. |
src/test/java/org/openrewrite/maven/CheckstylePropertiesTest.java
Outdated
Show resolved
Hide resolved
Hi @SaptarshiSarkar12 ; Appreciate your enthusiasm to get this going! I've not had time yet to dive into what would be needed next; I did push two small changes to hopefully get the test going, after seeing the tests previously failed with
Sadly now there's a different puzzling error. |
Now finally we fail with
Time to work in this earlier suggestion #699 (comment) ; Would you want to try your hand at that @SaptarshiSarkar12 ? |
@timtebeek Thank you for spending your valuable time! Sorry, I was very busy the last three days. So, I couldn't reply. |
@timtebeek Please tell me if any further changes in the missing files are required 😃. |
@timtebeek Please reply 😃. |
Hi yes sorry I'm swamped with work and other reviews at the moment. This PR seems like it would take more effort on my side to help guide you through, which I'm having trouble fitting in on the short term. There's were some earlier hints here and here, but I don't know if that's enough for you to try your hand at changing the actual code too in addition to the test you've already added. |
…to 699-add-checkstyle-props-support
@timtebeek I was busy in some other works the last month. I went through some parts of the Do you know how to fix that? rewrite-maven-plugin/src/main/java/org/openrewrite/maven/ConfigurableRewriteMojo.java Lines 265 to 276 in 11bca1e
|
@timtebeek Can you please help me to fix the above problem? |
Based on the screenshot it looks like you need to rename your folder from CheckstylePropertiesTest to CheckstylePropertiesIT. |
@knutwannheden Thank you for the help 😄! |
I've renamed the test resources to line up with the test class name, as required by the integration test framework. Unfortunately it seems there's now some conflicts with the main branch, after merging this recent support for inline config: |
Thank you 😄. But, there's another error coming up as posted in this comment. Can you help me fix that error?
I have resolved the conflicts and merged the changes successfully. |
@timtebeek Can you please tell me how do I fix that error? |
I've pushed up a change to fix the test for now @SaptarshiSarkar12 ; hope that helps you on your way! |
Thank you @timtebeek 😄. I'll try if it works, and no errors are found. Will certainly let you know 🙃. |
@timtebeek Sorry for the delay. I was quite busy in these few months. So, I couldn't get time. I tried running the tests today and it worked successfully 🎉. Here's a screenshot for your reference 😄. Thank you for the fix. |
@timtebeek I wanted to know how I can print the content of the checkstyle config that I modified in this code block. I tried this one, but it didn't work. Also, the output of // Print out each line of the resulting string
getLog().info("Checkstyle config file contents:\n" + checkstyleConfig); |
Hi @SaptarshiSarkar12 I suppose you want to print as a form of debugging? Might be better to attach an actual debugger if at all possible. We don't use logging much, but you can assert a particular message was logged with the pattern seen here. rewrite-maven-plugin/src/test/java/org/openrewrite/maven/BasicIT.java Lines 43 to 47 in e8b5855
|
Yeah, I wanted to test if the checkstyle config string is modified or not.
@timtebeek But I wanted to see what the content of the checkstyle config string is. How do I do that? |
You can either create an assertion that fails (does the |
There's no method named @timtebeek I tried to see if I can get anything I logged previously in the I tried setting breakpoints but they didn't get triggered during the execution of the test via IntelliJ. No notification for ignoring a breakpoint (the usual behaviour of IntelliJ) was observed. |
@timtebeek Please reply 😄. |
Hi @SaptarshiSarkar12 ! Apologies; it's hard to get to everything. As far as I can tell breakpoints indeed do not work with the tst framework we use for the plugin, so that will complicate the work that you're doing. You can see how assertions on the logged output are handled in this recent example. rewrite-maven-plugin/src/test/java/org/openrewrite/maven/RewriteRunIT.java Lines 48 to 57 in d86d22c
A similar approach could help you in working through your plugin changes there. |
Hi Tim 👋! |
What's changed?
Fixes #699
Anyone you would like to review specifically?
@timtebeek
Checklist