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

Changed update! :pre to just non-nil check id #19

Merged
merged 4 commits into from Dec 11, 2017

Conversation

AndreTheHunter
Copy link
Contributor

@AndreTheHunter AndreTheHunter commented Dec 11, 2017

Thanks for contributing to Toucan. Before open a pull request, please take a moment to:

  • Ensure the PR follows the Clojure Style Guide and the Metabase Clojure Style Guide.

  • Tests and linters pass. You can run them locally as follows:

    lein test && lein lint
    

    (CircleCI will also run these same tests against your PR.)

  • Make sure you've included new tests for any new features or bugfixes

  • New features are documented, or documentation is updated appropriately for any changed features.

  • Carefully review your own changes and revert any superfluous ones. (A good example would be moving words in the Markdown documentation to different lines in a way that wouldn't change how the rendered page itself would appear. These sorts of changes make a PR bigger than it needs to be, and, thus, harder to review.)

    Of course, indentation and typo fixes are not covered by this rule and are always appreciated.

  • Include a detailed explanation of what changes you're making and why you've made them. This will help us understand what's going on while we review it.

Once you've done all that, open a PR! Make sure to at-mention @camsaul in the PR description. Otherwise I won't get an email about it and might not get review it right away. :)

Thanks for your contribution!

Copy link
Contributor Author

@AndreTheHunter AndreTheHunter left a comment

Choose a reason for hiding this comment

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

Issue: #20

@@ -11,6 +11,7 @@
[honeysql "0.8.2"]]
:javac-options ["-target" "1.7", "-source" "1.7"]
:aliases {"bikeshed" ["bikeshed" "--max-line-length" "130"]
"lint" ["do" ["eastwood"] "bikeshed" "docstring-checker"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added lein lint to make it easier to run the commands locally

@@ -379,7 +379,7 @@
honeysql-form)))))

(^Boolean [model id kvs]
{:pre [(integer? id) (map? kvs) (every? keyword? (keys kvs))]}
{:pre [(some? id) (map? kvs) (every? keyword? (keys kvs))]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix.
We use UUID ids

@@ -15,3 +15,4 @@ pom.xml.asc
profiles.clj
/.env
/build.xml
/*.iml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntelliJ project file. We use https://cursive-ide.com/

@@ -1,4 +1,4 @@
(defproject toucan "1.1.0"
(defproject toucan "1.1.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump version number

@@ -484,7 +484,7 @@

(extend ~instance
~@(mapcat identity (merge-with (fn [this that] `(merge ~this ~that))
`{toucan.models/IModel models/IModelDefaults
`{toucan.models/IModel toucan.models/IModelDefaults
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found this bug when writing a marco that uses defmodel

@camsaul
Copy link
Member

camsaul commented Dec 11, 2017

Thanks @AndreTheHunter, looks good to me.

@camsaul camsaul merged commit 16fbcd9 into metabase:master Dec 11, 2017
@AndreTheHunter AndreTheHunter deleted the fix-update-pre branch December 11, 2017 23:43
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.

None yet

2 participants