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

Support specified files at the command line #12

Closed
1 of 2 tasks
Nayacco opened this issue Feb 24, 2021 · 16 comments · Fixed by #16
Closed
1 of 2 tasks

Support specified files at the command line #12

Nayacco opened this issue Feb 24, 2021 · 16 comments · Fixed by #16
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Nayacco
Copy link
Contributor

Nayacco commented Feb 24, 2021

💡 Describe the solution you'd like

java -jar /path/to/ktfmt-<VERSION>-jar-with-dependencies.jar [--dropbox-style] [files...]

Ktfmt supports specifying files that need to be formatted. I want this feature when I use the command line, maybe it is:

gradlew ktfmtFormat -Dglob=[files...]

My actual usage scenarios is that I have changed five files, and want to commit two of them. I registered a git hook to format the file to be committed. So I just want to format two files.

You can reference this issue: How to setup spotless for a git pre commit hook that only check changed files

🤚 Do you want to develop this feature yourself?

  • Yes
  • No
@cortinico
Copy link
Owner

This doesn't work out of the box, mostly because Gradle doesn't have notion of Git and the whole idea behind the tool is to give you tasks that run autoformats for every source sets you have.

I have two questions to challenge this:

  1. Why don't you use ktfmt directly from the pre-commit hook? If you have gradlew ktfmtFormat in a pre-commit hook you'll end up paying the configuration time for your Gradle build, making the pre-commit way slower.

  2. Have you considered using one of the script from the issue you linked (like this one)? Ktfmt-gradle has the concept of UP-TO-DATE files so it will only reformat the files that are actually modified.

@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 24, 2021

This doesn't work out of the box, mostly because Gradle doesn't have notion of Git

I just want the ability to specify files in the command line. I'll use git diff to concatenate a file path string in the bash and then takes the string as a command-line argument. So it does not require a Git environment.

Why don't you use ktfmt directly from the pre-commit hook? If you have gradlew ktfmtFormat in a pre-commit hook you'll end up paying the configuration time for your Gradle build, making the pre-commit way slower.

I didn't want to use the ktfmt.jar directly, but instead introduced it as a plug-in so that it would be easy to upgrade the plug-in later. I think it's fast when I use the --daemon argument. And this is a common usage in frontend(Web), just like this popular project vue-element-admin

https://github.com/PanJiaChen/vue-element-admin/blob/33a93a12b4dc6429dbe91f43ef17fb76236f2d6c/package.json#L96-L106

It automatically registers a Git Hook by using husky and use lint-staged to filter out the files to be committed, as it mentions in the README: Ultimately you only want to lint files that will be committed.

Have you considered using one of the script from the issue you linked?

Yes, I've already tried, ktfmt-gradle didn't work. What I want to achieve is to automatically format the committed files, so as I said above, the other three files don't need to be formatted. I'm a Web developer, and since this approach is widely used in the Web space, so I mention this feature request.

@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 24, 2021

Here is an other example from Facebook's formatting tool prettier

@cortinico
Copy link
Owner

I see your point. So I'm thinking about an approach that could look like:

tasks.register<KtfmtFormatTask>("ktfmtPrecommit") {
    setSource(project.files(project.findProperty("glob").toString().split(":").toTypedArray()))
}

You would then call it with:

gw ktfmtPrecommit -Pglob=/opt/file1.kt:/opt/file2.kt

There are a couple of changes needed to make this possible.

@cortinico cortinico added enhancement New feature or request help wanted Extra attention is needed labels Feb 24, 2021
@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 25, 2021

I understand your intention to customize to achieve flexibility or generality,but I doubt whether to do this, based on the following two reasons:

  1. This plugin is a package of ktfmt, so this itself is one of the functions of ktfmt, and the functions of ktfmt are very limited, so is there really a need for customization?
  2. Do you have a plan to be compatible with maven, or maybe someone else will submit a PR to be compatible with maven, using too many gradle features will make compatibility difficult.

So maybe a fixed api would be better, but this is just an immature suggestion, I don’t know much about gradle and maven.

@cortinico
Copy link
Owner

This plugin is a package of ktfmt, so this itself is one of the functions of ktfmt, and the functions of ktfmt are very limited, so is there really a need for customization?

The rationale behind my suggestion is that I expose the Tasks classes that can be used externally to make custom ktfmt tasks if needed. So that's definitely aligned with the Gradle API and I believe that should be supported to some extent

Do you have a plan to be compatible with maven, or maybe someone else will submit a PR to be compatible with maven, using too many gradle features will make compatibility difficult.

No plan to make this work with Maven, at least from my side

@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 26, 2021

If just support Gradle, this is a great solution.

@cortinico
Copy link
Owner

I've done some work on this front but with not much success. Currently Gradle expects that the source of a SourceTask is a directory, that doesn't play well with our use case of being a list of files.

See: gradle/gradle#15679

@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 27, 2021

Yes, It seems that it can only do by include api

setSource(
    srcDir(project.rootDir).include(project.findProperty("glob").split(":"))
)

@cortinico
Copy link
Owner

Yes, It seems that it can only do by include api

I'm unsure I follow you. Your script does not compile for me:

* What went wrong:
Script compilation errors:

  Line 26:         srcDir(project.rootDir).include(project.findProperty("glob").split(":"))
                   ^ Unresolved reference: srcDir

@Nayacco
Copy link
Contributor Author

Nayacco commented Feb 28, 2021

I got it wrong. Forget it

@Nayacco
Copy link
Contributor Author

Nayacco commented Mar 5, 2021

#!/bin/sh
# git config core.hooksPath hooks
echo '[git hook] executing gradlew ktfmtFormat:write before commit'

FILES=$(git diff --cached --name-only --diff-filter=ACMR "*.kt" | sed 's| |\\ |g')
[ -z "$FILES" ] && exit 0

# Prettify all selected files
JOIN_FILES=$(echo "$FILES" | sed ':a;N;$!ba;s/\n/:/g')
./gradlew ktfmtPrecommit --include-only="$JOIN_FILES" --daemon

# store the last exit code in a variable
RESULT=$?

# Add back the modified/prettified files to staging
echo "$FILES" | xargs git add

exit $RESULT

Share my currently available git hook scripts

@cortinico
Copy link
Owner

I'm looking into tackling this issue with #16

@Nayacco
Copy link
Contributor Author

Nayacco commented May 22, 2021

I have updated the git hook script to format only the submitted files.

@cortinico
Copy link
Owner

I have updated the git hook script to format only the submitted files.

So I suppose it's working fine now? :)

@Nayacco
Copy link
Contributor Author

Nayacco commented May 22, 2021

Yes, I've already tested it. Thank you very much for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants