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

Added models/primary-key #53

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Conversation

axrs
Copy link
Contributor

@axrs axrs commented Jan 21, 2019

Added in support for changing the primary key, #3

  • Added IModel/primary-key
  • Added snapshot job to CircleCI and publish with groups
  • Improve delete! performance
  • Improve exists? performance and resolve model first
  • Made get-inserted-id public

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. See the README for more details on running tests.

  • 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!

* Added IModel/primary-key
* Added docstring for model/primary-key. Added snapshot job to CircleCI
* Improve delete! performance
* Improve exists? performance and resolve model first
* Made get-inserted-id public
* Made resolve-primary-key public
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Can you please carefully go thru your PR and remove all the changes to the CI config and other unrelated changes. It looks to me like you might just need to rebase your commit off latest master.

@AndreTheHunter
Copy link
Contributor

AndreTheHunter commented Jan 23, 2019

Can you please carefully go thru your PR and remove all the changes to the CI config and other unrelated changes. It looks to me like you might just need to rebase your commit off latest master.

The changes in the CircleCI yaml config allows allows us to deploy our own fork. See https://circleci.com/gh/jesims/workflows/toucan deploying to https://clojars.org/io.jesi/toucan. I've made that configuration environment variable driven as not break anything in metabase/toucan. Since we need our changes in production, we can't always wait for it to be merged into the upstream.

I've checked, and this branch is up to date with master. I'd recommend configuring this repository and setting master as a protected branch. Go to https://github.com/metabase/toucan/settings/branches. See https://help.github.com/articles/enabling-required-status-checks/ for more info.

Happy to discuss any changes/alternatives 😃

@camsaul
Copy link
Member

camsaul commented Jan 24, 2019

I don't want to merge those changes you've made to the CI config or project.clj into the core Toucan repo

@@ -23,7 +23,8 @@
:exclusions [org.clojure/clojure]]
[lein-bikeshed "0.5.1"]
[lein-check-namespace-decls "1.0.1"]
[lein-expectations "0.0.8"]]
[lein-expectations "0.0.8"]
[lein-shell "0.5.0"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add missing lein-shell. It's used in start-db and stop-db aliases

@@ -11,7 +11,7 @@
[honeysql "0.9.4"]]
:javac-options ["-target" "1.7", "-source" "1.7"]
:aliases {"bikeshed" ["bikeshed" "--max-line-length" "118" "--var-redefs" "false"]
"lint" ["do" ["eastwood"] "bikeshed" "docstring-checker" "check-namespace-decls"]
"lint" ["do" ["eastwood"] ["bikeshed"] ["docstring-checker"] ["check-namespace-decls"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.
The args "docstring-checker" and "check-namespace-decls" were being passed into "bikeshed".

Separate commands need to end in a comma or be in separate vectors. e.g. https://github.com/technomancy/leiningen/blob/master/sample.project.clj#L242
See https://github.com/technomancy/leiningen/blob/master/src/leiningen/do.clj#L21

Copy link
Member

Choose a reason for hiding this comment

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

yup, makes sense. Thanks

@AndreTheHunter
Copy link
Contributor

@camsaul Those changes wouldn't break anything but OK.
I've added some comments outlining the changes to project.clj

@camsaul
Copy link
Member

camsaul commented Jan 24, 2019

Thanks @AndreTheHunter. Looks good.

@camsaul camsaul merged commit b438f1e into metabase:master Jan 24, 2019
@camsaul
Copy link
Member

camsaul commented Jan 24, 2019

Merged. I'll update documentation and other things in a few days and push out a new release if all looks good

@camsaul
Copy link
Member

camsaul commented Jan 29, 2019

Pushed out release 1.11.0 which includes this PR. Thanks again

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.

3 participants