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.

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

Add support for creating tasks to format script files (#337) #382

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Roggstars
Copy link

🚀 Description

Following #337, I have added two tasks on the project level: ktfmtCheckScript and ktfmtFormatScript. These tasks will evaluate the formatting of top-level script (*.kts) files, like build.gradle.kts.

📄 Motivation and Context

Currently, script files are ignored by the plugin. Having a consistent formatting between those and source files would be great.

🧪 How Has This Been Tested?

I have added (and adjusted) some integration and unit tests. The old tests actually had some formatting issues in the script files that were used to set up the project in a temp directory. I have separated those changes into separate commits for better legibility during review. I will happily squash them into the primary feature commit, if necessary and desired by you.

I have also published the plugin to my local maven and applied it to

  • an Android project
  • a JVM project
  • a multi-module KMP project (with limitations, see below)

Limitations
As documented in the README, ktfmt is not supposed to be applied to the top-level build.gradle.kts. Therefore, the top-level script files are currently not formatted. Is this a limitation of the plugin itself? Do we want to be able to apply the plugin on the top-level (might be useful now that we actually want to format files that are there).

📦 Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@simonhauck
Copy link
Collaborator

Thanks for this amazing PR, i will have definitely a look at it later.

There is still one test failing. Could you have a look @Roggstars ?

@Roggstars
Copy link
Author

I'll check it out, thought I had run the preMerge task locally...

Copy link
Owner

@cortinico cortinico 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 sending this over!

You still have a failing test though:

KtfmtCheckTaskIntegrationTest > check task should detect the source and test files in a flattened project structure() FAILED
    org.gradle.testkit.runner.UnexpectedBuildFailure at KtfmtCheckTaskIntegrationTest.kt:312

Comment on lines 72 to 73
val checkTaskName = "${TASK_NAME_CHECK}Script"
val formatTaskName = "${TASK_NAME_FORMAT}Script"
Copy link
Owner

Choose a reason for hiding this comment

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

nit: I'm wondering if those tasks should be pluralized instead:

Suggested change
val checkTaskName = "${TASK_NAME_CHECK}Script"
val formatTaskName = "${TASK_NAME_FORMAT}Script"
val checkTaskName = "${TASK_NAME_CHECK}Scripts"
val formatTaskName = "${TASK_NAME_FORMAT}Scripts"

Copy link
Author

Choose a reason for hiding this comment

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

Agreed and resolved in 4bd1fa6

@Roggstars
Copy link
Author

Roggstars commented Nov 29, 2024

I am still not 100% sure why the Windows machine keeps complaining about that single test. It fails because the build.gradle.kts file the test creates and appends is malformatted. I suspect this might have to do with different line separators on the different operating systems, but I am not 100% sure yet. I will check this out on my Windows machine and push a fix.

Edit: also, sorry for my force-pushes. Wasn't aware of all the messages this would create on the original issue. Will continue working on a branch in my fork now and only push to main when resolved. :)

@cortinico
Copy link
Owner

Edit: also, sorry for my force-pushes. Wasn't aware of all the messages this would create on the original issue. Will continue working on a branch in my fork now and only push to main when resolved. :)

No problem at all 👍 I'll mark this as draft and please change it back to ready-for-review when you're done

@cortinico cortinico marked this pull request as draft November 29, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants