update asynctest to 0.5.1

[markspanbroek]

No description provided.

[dryajov]

LGTM, but why do we need to import chronos/unittest explicitly?

[markspanbroek]

why do we need to import chronos/unittest explicitly

Good question. The asynctest code became rather hard to maintain, because it consisted of code that just happened to be compatible with both chronos and asyncdispatch. This already lead to unwanted warnings in some cases, and became even harder to maintain with the recent chronos changes w.r.t. exceptions. The solution was to have different code for asyncdispatch and chronos, but that requires an explicit choice when importing asynctest (codex-storage/asynctest#11).

[gmega]

Hey @markspanbroek I ended up folding this into my Chronos V4 branch cause there I actually cannot compile things without this in place. I'm also keeping it up-to-date with the latest master so, if we want, we can bundle this bump with the V4 update.

[dryajov]

Lets get this one merged as well, regardless of it being in #673

[gmega]

Yeah, that's probably wise at any rate as it will make the V4 merge smaller. I'm going to update this and ready it for merge.

[gmega]

Easy peasy lemon squeezy. 🙂

[gmega]

LGTM!

[dryajov]

One thing that we should do, is just create a wrapper around the import and import that directly instead of explicitly importing chronos.

[gmega]

One thing that we should do, is just create a wrapper around the import and import that directly instead of explicitly importing chronos.

You mean to shorten the path? As in a package alias so you don't have to type import pkg/asynctest/chronos/unittest? I can do that, but not sure I prefer having to compute relative import paths in my head (as this would have to go under e.g. /tests/asynctest.nim) over typing a longer-but-always-the-same import line.

[gmega]

OK, I've updated it to use a wrapper, and it sort of confirms my point: it was an absolute pain to go through 40+ files and figure out the relative import path in each (and IMHO it looks uglier 🙂). I also ran into issues as Nim will import the wrong package if you do import ./asynctest but that relative path happens to be wrong (which doesn't happen when you import the package directly).

But now that it's done, the extra level of indirection will at least make it easier to swap the import in the referred package. I also don't anticipate this will be as painful when updating incrementally.

[markspanbroek]

Github won't let me approve because I started the PR, but feel free to merge at any time @gmega.

[gmega]

Merging is blocked for me too, @markspanbroek! It requires reviews, perhaps cause I did changes after you. Can you approve, @dryajov or @elcritch?

[dryajov]

LGTM