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

Include project.clj by default for lein plugin #280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jacobemcken
Copy link

Since the file project.clj is specific to Leiningen projects it seem like a good default.

Resolves: #279

@weavejester
Copy link
Owner

Thanks for the PR!

Why did you choose concat over cons here?

Also, can you change the commit message to:

Update Lein plugin to always include project.clj

Fixes #279.

I think the "by default" is misleading, because in this case it can't be changed. If this turns out to be an issue, we can add in an option to allow it to be omitted.

Have you tested to ensure that this plugin change works with both check and fix?

@jacobemcken jacobemcken force-pushed the feature/include_project.clj branch from 59ddbdd to 5801e3b Compare December 29, 2022 13:25
@jacobemcken
Copy link
Author

jacobemcken commented Dec 29, 2022

Why did you choose concat over cons here?

My lack of knowledge.

I think the "by default" is misleading, because in this case it can't be changed.

Either I don't understand meaning of your comment or I disagree. As soon as you provide an alternative configuration in the project.clj file, like:

...
    :cljfmt {:paths ["src"]}
...

The project.clj file is no longer include. What do you mean by "it can't be changed"?

I had only tested with check initially, but now I have also successfully tested it with fix 👍
Thanks for the reminder.

@weavejester
Copy link
Owner

As soon as you provide an alternative configuration in the project.clj file, like:

...
    :cljfmt {:paths ["src"]}
...

The project.clj file is no longer include

I don't think this is correct:

leiningen.cljfmt=> (format-paths {:source-paths ["src"], :test-paths ["test"], :cljfmt {:paths ["src"]}})
("project.clj" #object[java.io.File 0x2ac11170 "src"])

You cons the string "project.clj" onto the list regardless of what is set in :paths. You've also put the string in the wrong place, as its not checked to see whether it exists (a small possibility, but a possibility), and it's not converted into a file instance.

Since the file project.clj is specific to Leiningen projects it seem
like a good default.

Fixes: weavejester#279
@jacobemcken jacobemcken force-pushed the feature/include_project.clj branch from 5801e3b to 7ada90e Compare January 18, 2023 12:51
@jacobemcken
Copy link
Author

... and it's not converted into a file instance.

The reason why it worked as a string regardless, is that both check and fix parses the list of "paths" through find-files which applies io/file again:

(defn- find-files [{:keys [file-pattern]} f]
(let [f (io/file f)]

Thankfully, layering io/file on itself like (io/file (io/file "some_file.txt")) just returns a file object #object[java.io.File 0x2ac11170 "some_file.txt"].

I've moved "extending paths with the project file" earlier, which also required changing how the filter works. An alternative would be to only check for (.exists %). Is this what you had in mind?

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.

Do you want the project.clj file to be included by default for lein projects?
2 participants