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

Fix build/release issues #228

Closed
wants to merge 16 commits into from
Closed

Conversation

chrisbetz
Copy link

@chrisbetz chrisbetz commented Apr 18, 2019

Hi,

for building a release containing your latest commits/PRs (#222), I needed to setup and build my own version of kibit.

I fixed some issues you might want to copy over to your project.

  • Use git version tagging (using lein v)
  • Automated creation of version file to reference (so it's just a lein release :patch to actually release a new version)
  • Fixed JDK11 build on TravisCI
  • Added "get started building kibit/lein-kibit" section to README
  • Added / fixed badges in README
  • Updated CHANGELOG.md (and using lein changelog for managing during release)
  • Updated dependencies

Feel free to take whatever you like and dismiss everything else.


This change is Reviewable

Dr. Christian Betz added 16 commits April 17, 2019 12:47
Run deps only on kibit project, not on lein kibit.
TravisCI has problems building on OpenJDK 11 (which I do not have on AdoptOpenJDK 11). Maybe the problems are gone after upgrading (which I tend to like anyhow), so check.
- Test on OracleJDK 11
- Add Changelog plugin
- Update Changelog
Add Badges and an Introduction on how to build Kibit
@chrisbetz chrisbetz changed the title Fixing several issues Fix build/release issues Apr 18, 2019
Copy link
Member

@danielcompton danielcompton left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR here, and splitting the commits up into small chunks. I'm slightly wary of taking on two new lein plugins for this, but will give it some thought. I know @arrdem has more experience with lein-monolith than I.

I've put some notes here on the PR, but please don't feel like you need to address them, it's mostly for myself.

- "pushd kibit"
- "lein deps"
- "popd"
- "LEIN_USE_BOOTCLASSPATH=no lein cache-version"
Copy link
Member

Choose a reason for hiding this comment

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

I think LEIN_USE_BOOTCLASSPATH=no could be set once as an environment variable?

Copy link
Member

Choose a reason for hiding this comment

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

Also, if we are going to add a makefile, can we use those make commands here?

### Additions
* Clojurescript/Cljx support (cljc support coming soon). This just works™, kibit will pick up your source paths from your `project.clj`'s `:source-paths`, `[:cljsbuild :builds]`, and `[:cljx :builds]`.
* Non-zero exit codes. Kibit now exits non-zero when one or more suggestions are made. This is particularly useful for those running checks in a CI environment.
* You can now run kibit on any Clojure project without a project.clj file. Just call `lein kibit` with any number of files and folders and it will inspect the Clojure files contained within.

[Unreleased]: https://github.com/gorillalabs/kibit/compare/v0.3.0...HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Can you change gorrillalabs -> jonase here, and in the other places it was used?

(slurp
(io/resource
"jonase/kibit/VERSION")))]]
kibit-version (clojure.edn/read-string
Copy link
Member

Choose a reason for hiding this comment

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

Can you match indentation here?

"jonase/kibit/VERSION")))]]
kibit-version (clojure.edn/read-string
(slurp (io/resource "version.edn")))
kibit-project `{:dependencies [[gorillalabs/kibit ~(:version kibit-version)]]
Copy link
Member

Choose a reason for hiding this comment

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

Would be neat if lein-v could also print out the group and artifact ID, so it didn't need to be hard-coded here.

(io/resource
"jonase/kibit/VERSION")))]]
kibit-version (clojure.edn/read-string
(slurp (io/resource "version.edn")))
Copy link
Member

Choose a reason for hiding this comment

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

If any user of kibit has a file called version.edn at the root of their classpath then there will be conflicts between this one and that one. It would be good to be able to move it to jonase/kibit/version.edn to avoid the risk of conflicts.

@dijonkitchen
Copy link

Any update on this? I think it fixes #229

@danielcompton
Copy link
Member

I've got a bunch of review comments that are blocking merging of this PR. I'll do a separate release though.

@NoahTheDuke
Copy link
Collaborator

Closing due to lack of movement.

@NoahTheDuke NoahTheDuke closed this May 9, 2024
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.

4 participants