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

Add some tests for role rule fns #196

Merged
merged 5 commits into from
Sep 15, 2022
Merged

Add some tests for role rule fns #196

merged 5 commits into from
Sep 15, 2022

Conversation

cap10morgan
Copy link
Contributor

This test fails on db main HEAD, but is fixed by fluree/db#199.

@cap10morgan cap10morgan requested a review from a team September 14, 2022 19:49
Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

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

lgtm.

@@ -1,7 +1,10 @@
{:deps {org.clojure/clojure {:mvn/version "1.11.1"}
org.clojure/data.xml {:mvn/version "0.2.0-alpha7"}
com.fluree/alphabase {:mvn/version "3.3.0"}
com.fluree/db {:mvn/version "2.0.0-alpha6"}
;com.fluree/db {:mvn/version "2.0.0-alpha6"}
Copy link
Contributor

Choose a reason for hiding this comment

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

super miner, but it looks like these commented deps stayed in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's kind of nice to just hit the comment / uncomment toggle in the editor to switch among these. Happy to remove if others prefer, though.

(throw result)
result))

(defn printlnn
Copy link
Contributor

Choose a reason for hiding this comment

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

😆


db (fdb/db (:conn test/system) ledger {:syncTo block})

;; Can edit own chat messages
Copy link
Contributor

Choose a reason for hiding this comment

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

a little pedantic, but maybe we could organize these chunks with testing blocks instead of comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you nest those?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a shot but it gets kind of messy with the re-use of the let values I'm doing (b/c you have to break out of the current let vec then go back into a new one). I think I'll stick w/ this approach for now. But future refactorings are welcome! :)

@cap10morgan
Copy link
Contributor Author

Thanks for the review, @zonotope!

@cap10morgan cap10morgan merged commit 91882c0 into main Sep 15, 2022
@cap10morgan cap10morgan deleted the fix/permissions branch September 15, 2022 20:05
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.

2 participants