frioux / DBIx-Class-Helpers

https://metacpan.org/pod/DBIx::Class::Helpers
20 stars 38 forks source link

Add support for query condition to Helper::ResultSet::Shortcut::ResultsExist #102

Closed dboehmer closed 4 years ago

dboehmer commented 4 years ago

I love DBIx-Class-Helpers and very often use some of its components! Just recently I've found that is also provides an EXISTS helper. Yay, thanks for your great work on this dist! :+1:

I suggest passing the 1st argument to results_exist() along to have a similar usage like search().

In my project Coocook I added my own custom exists() method and I'd like to migrate to your code. But I often pass a query to my exists() like here.

For reference: My method is implemented in Coocook::Schema::ResultSet

Beware:

dboehmer commented 4 years ago

When I look at the recent commits it looks like Pg tests are being worked on. I hope the test failures on Travis CI have nothing to do with my changes.

frioux commented 4 years ago

My plan for the shortcuts was to always have all the doc in one place: https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::Shortcut; if something is missing or unclear there, it should be added there. I'd be ok with a link to that, or automation to have the doc in both places, but I don't want to support doc in two places unless that's automated.

As for the search args, that's fine.

dboehmer commented 4 years ago

I rebased this on top of your master. Please review again.

frioux commented 4 years ago

The above says tests failed; please fix that and then I can merge. Thanks!

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.002%) to 98.564% when pulling 10ca0e9ceab9e514ce97b4fc3e409c9374deb66e on dboehmer:results-exist into 9d355d44c9a4cc875daef4f6922fa4c4f44aeabc on frioux:master.

dboehmer commented 4 years ago

I pushed a fix. Do you want me to squash the fix?

I also found that the logic was broken when I used 5cfc109 in my actual project. The tests in my software started to fail because results_exist returned wrong check results. The failed test here didn’t actually detect the logic error but Postgresql didn’t accept the query. I hope that’s acceptable as it is now fixed. Tests in my software pass now, too.

frioux commented 4 years ago

Yes, please squash.

dboehmer commented 4 years ago

GitHub displays the Travis CI tests as still being in progress but when I look at the Travis CI page they've completed and passed. So you might already merge.

frioux commented 4 years ago

Merged and will release shortly, thanks!

frioux commented 4 years ago

released