Motiva-AI / clj-sqs-extended

Client library for AWS SQS with support for large messages, up to 2GB. This is a Clojure wrapper for https://github.com/awslabs/amazon-sqs-java-extended-client-lib
MIT License
3 stars 0 forks source link

Milestone 2 code review A #31

Closed Quantisan closed 4 years ago

Quantisan commented 4 years ago

I'll start with reviewing core.clj here since that's the API. In decreasing priority:

  1. i want to see many more unit tests for handle-queue because that's a API fn. normal cases, coverage on expected failure cases (non-existent queue when initializing, losing network connection within the loop after starting, etc.)

  2. public vars in core should only relate to the API. receive-loop shouldn't be there (unless you want it available?). Conversely, send-message and send-fifo-messageshould be included in this ns, probably minimally with just a(def send-mesage sqs-ext/send-message)`.

  3. what's that extra bracket on L143? Bonus. clj-sqs-extended.example should be moved under test/

  4. receive-loop fn is too long to read. Single responsibility principle. Refactor out a bunch of small fns so that the length of this fn isn't more than ~80 lines. Similarly, for handle-queue. This might take some thinking to refactor so perhaps we can punt it for later and focus on finishing the milestone for now.

  5. local development workflow appears to be hindered by the slow tests. executing circleci local takes a few minutes to run. ideally we want local dev to finish tests in seconds. one quick way might be to tag :integration tests requiring localstack from the rest and ensure a clean separation via mocks or by design to limit :integration dependencies in tests as much as possible. That way, we can just run lein test without dependencies during local development and even run tests in REPL. A second solution is to use docker-compose and keep the dependent services running in the background. This latter approach is what we usually use in Motiva.