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

Make Lemur suitable as a library #31

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jeroenvandijk
Copy link

Guided by #23 I've continued the work of @jimdowning.

The code should be suitable now to use a library, although it is a bit ugly still due to the use of one dynamic var. This was the easiest option for now. I hope to do some more iterations later on. I say "should" because I still need to test it from a project. The tests I could run without the need for AWS access were succeeding, but not sure about the others.

For short-term future work I see the following:

  • Have library functions that take the context as an argument
  • Move dsl functions in lemure.core to a separate namespace

Happy to hear your first feedback on these changes

(defn quit-by-error [ex]
(quit* (:data (ex-data ex))))

(defn error [& {:keys [msg cmdspec exception exit-code] :as data :or {exit-code 0}}]
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think you need to mention all the explicit keys here.

Copy link
Author

Choose a reason for hiding this comment

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

True. I'll remove them.

@jeroenvandijk
Copy link
Author

@mlimotte I've made corrections based on your suggestions.

[ec2 :as ec2]
[s3 :as s3]
[emr :as emr]]
[lemur [core :as l]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line's indentation of [core] is inconsistent with the line below

@leon-barrett
Copy link
Contributor

Aside from me being picky about indentation, this seems great. @aykuznetsova said she might be able to test this out with our use cases, so let's wait a bit to merge for her verification.

@jeroenvandijk
Copy link
Author

I've created a test environment to test the new changes https://github.com/jeroenvandijk/lemur-test-ground

The old functionality seems to be okay. The new functionality is not complete yet and needs some more iterations, or at least some investigation.

@mlimotte
Copy link
Contributor

We should update CHANGES.txt also.

And, eventually, the version number and the README-- but maybe we do those post-commit.

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