Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-617] - Improve Clojure styles with cljfmt tools revise #11537

Merged
merged 8 commits into from Jul 3, 2018
Merged

[MXNET-617] - Improve Clojure styles with cljfmt tools revise #11537

merged 8 commits into from Jul 3, 2018

Conversation

agilecreativity
Copy link
Contributor

@agilecreativity agilecreativity commented Jul 3, 2018

Description

Use cljfmt to keep styles of the project consistent.
This include the updated version that have a cleaner commit history and rebase on the latest changes from @gigasquid.

Please help review.
Thanks

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • [ X] The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • [ X] Changes are complete (i.e. I finished coding on this PR)
  • [ X] All changes have test coverage:
  • [X ] Code is well-documented:
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • [ X] To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Remove extra file vi ci-test.sh from the project
  • Add steps to run the new script to README.md file
  • Add lein-cljfmt to the plugins vector for all Clojure projects
  • Add the lein-cljfmt-check to each project and the base directory to be run in batch
  • Run the lein-cljfmt-fix to fix the style of the project excluding some files when necessary

Comments

  • These scripts will make it easier for new contributor to keep using the same styles going forward
  • The bash flag from lein-cljfmt-check is intentionally omit the -e to allow the script to continue

- Add lein-cljfmt-check to each project
- Add lein-cljfmt-fix to each project
- Run $MXROOT/contrib/clojure-package/lein-cljfmt-fix
- Run $MXNET_ROOT/contrib/clojure-package/examples/lein-cljfmt-fix
@@ -0,0 +1,22 @@
#!/usr/bin/env sh
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about moving this script to a bit more top-level directory and then applying it recursively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcoabreu
I did not think of this, but yes this is a great idea indeed.

@agilecreativity
Copy link
Contributor Author

@gigasquid
Please help review and provide any feedback for improvement.
Thanks

@gigasquid
Copy link
Member

Thanks @agilecreativity for putting this together. I'll take a look later tonight.

Copy link
Member

@gigasquid gigasquid left a comment

Choose a reason for hiding this comment

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

Looks great! I really like the move to just the single top level script. It simplifies things :)

I ran an integration test locally and also tested out the formatting scripts. Everything looks fine.

Thanks again for the contribution 💯

@marcoabreu marcoabreu merged commit 40ae851 into apache:master Jul 3, 2018
@agilecreativity agilecreativity deleted the update-style-with-cljfmt-mxnet-617 branch July 3, 2018 23:41
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
…#11537)

* Remove 'vi ci-test.sh' file

* Add the scripts to reformat the style with cljfmt

- Add lein-cljfmt-check to each project
- Add lein-cljfmt-fix to each project

* Add lein-cljfmt to the plugins vector

* Add steps to keep style consistent to README.md

* Run lein-cljfmt-fix on the main src/test codes

- Run $MXROOT/contrib/clojure-package/lein-cljfmt-fix

* Run lein cljfmt fix on the example projects

- Run $MXNET_ROOT/contrib/clojure-package/examples/lein-cljfmt-fix

* Use only one script in the base directory.

- Thanks to @marcoabreu for the suggestion/review

* Minor update to kick off the new build
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants