Need code review

Welcome! Great that you’re creating a Hex package.

Let’s try to answer your questions:

1. Your naming scheme is correct. While any names for packages are of course allowed, the advised convention is to indeed use Foo.Bar if you wrote a package Bar that extends the more general package Foo. In cases there exists both a Bar and a Foo and you wrote some kind of compatibility-layer between them, the order doesn’t matter (so you could go for e.g. alphabetical order there). So the name Ecto.Shortcuts is great!

Arguably. Some people will agree, others won’t. I myself would say that it is nice to be able to shorten Repo-based things inside your Controllers, (and saying that you basically should never talk to the Repo anywhere else). If Ecto.Shortcuts is the nicest way of doing so, is again very much a matter of taste.

It does mean that your ‘model’ is again directly connected with the repository, which is something that Ecto itself explicitly didn’t do. If you’re for instance dealing with multiple repositories that you want to switch between (a common example of this is Sharding), this is a problem.

The most important thing that might be considered an ‘antipattern’ is that your code is still missing documentation. In Elixir, documentation is a first-class member of the language, and most people try to add documentation to their functions.

I don’t see any other obvious anti-patterns at first glance. A really helpful tool to increase the quality of your code is Credo, which tries to point out some anti-patterns and inconsistencies in your code.

4. The way you are using __using__/1 is correct .

Besides the fact that documentation is important, be sure to use Semantic Versioning for your package, because this means that when other people list a certain version as dependency requirement, their code will not break if you decide update your package.

Awesome! Thank you for your feedback!

Once I get all of the functionality working, I’ll start working on the documentation. One more question, how would the tests look like for it? I have a very limited experience with TDD, and I’m not exactly sure how would I go about writing tests for macros.

You’re very welcome!

Test Driven Development means that you first write tests and then fill in your code so they pass, which is not what you’re doing here. (Which is, of course, also a fine approach. Each way has its own advantages and drawbacks.)

The thing you probably want to test here, and also the way you can test the macro, is by simply creating a few modules that include the using Ecto.Shortcuts line. You could then test if the commands are properly passed on to the actual Repo when you call them on your test modules.

Another possible test is to test what happens if using Ecto.Shortcuts is added to a module that actually does not contain an Ecto schema.

If you need inspiration, feel free to pop open any of Elixir’s core libraries, such as for instance Ecto itself to see how they write tests there. Most of them are very understandable and of (at least in my opinion) a high quality.

And, especially when your modules start getting more complex, it might be worthwhile to check out José Valim’s post on using Mocks during testing.

The issue with the approach above is that you are coupling your schema with one particular database and the schema is designed to represent any data source, which may not even be a database in the first place. We explore some of those things here: Ecto’s insert_all and schemaless queries « Plataformatec Blog

My concern with using something such as shortcuts is that people will forget those basic tenets schemas and repositories were built on. So I would rather define modules such as MyApp.Accounts.get_user(id), which is the direction Phoenix is taking, without a need to tie repositories directly into schemas.

I’ve decided to go with the name Ecto.Rut

The intention here is to offer simple wrappers around Ecto.Repo methods and simplify common use-cases in Ecto. This can be a means to ease people into the “Ecto way” or just simply reduce code repetition. But I do agree with you; complicated statements should use Ecto’s rich and flexible DSL.

FYI theres another library that does about the same thing:
https://github.com/MishaConway/ecto_shortcuts

In any case, congratulations on your first package!

Those sound eerily familiar… is it from the README of my own project you seem to have ripped off and claimed as your own?

Anyways, I don’t want to get petty on this forum, but I had to call out what seems like plagiarism to me. Ironically, this thread was a good run down of the issues to address in own project since it was reviewing a proxy of it.

Before I knew about this, I was already in the process of adding complete test coverage and updating to support the new Phoenix idiom that Jose pointed out. Though I am slightly annoyed, it is good validation that I was working on the right upgrades.

Sorry to introduce myself to this community in such a negative light, but I hope to have a lot of positive things to contribute in the future.

I’ll be very honest @Misha, when I started writing the package I did not even know yours existed. The code I originally shared for review was this:

When I tried to publish it, it was only then that I realized ecto_shortcuts had already been taken. When Jose asked me about it I was unable to articulate what I wanted this package to do and ripped off two lines from your README (which i thought explained it pretty well). I’m very sorry for that, but I’d like you to compare the rest of the readme and source code. I hope you’ll see that my plagiarism ended at the two lines.

When I learn a new language, I try to publish scripts, write packages and use it in production and try to do as much of it as myself, that’s the only way I learn - it’s just that my english isn’t that good. This was exactly that. Again, I’m very sorry.