search

drashland / rhum

A test double library
https://drash.land/rhum
MIT License
91 stars 6 forks source link

Rhum.asserts has no autocompletion #63

Closed rodolphocastro closed 3 years ago

rodolphocastro commented 3 years ago

Description

When using the "shortcut" Rhum.asserts to build assertions we don't get intellisense support.

To Reproduce

Steps to reproduce the behavior:

  1. Import Rhum v1.1.4
  2. Attempt to access auto completion for Rhum.asserts
  3. Notice nothing relevant will show up
  4. If you type the correct function name the assertion will still work as expected

Expected behavior

I expected to get autocompletion support, the same we have for all Rhum 😅

Suggested Solution(s)

I've been able to "export" functions with autocompletion by using a interface + a const that complied to that interface.

This is the snippet that worked for me. However those functions are declared inside my module so I'm not sure if this would work for mapping Deno.test calls.

/**
 * Default functions for creating primitives with Dixture.
 */
export const dixtureFns: GenerationFunctions = {
  BigInt: createBigInt,
  Bool: createBoolean,
  Number: createNumber,
  String: createString,
  NamedString: <T>(prefix: keyof T) => createString(prefix.toString()),
  Int: () => createNumber(true),
  FutureDate: () => createDate(),
  PastDate: () => createDate(false),
};

The whole snippet can be found here

The following test suites also replicate the error (as of version v1.1.4 at least): dixture unit tests. Note: I attempted to get that autocompletion to work on deps.ts and on the other files as well. No such luck.

Screenshots

The following show my attempts to get some suggestions for Rhum.asserts

rhum asserts Code_1QnTwHE7Z3 Code_Tr1GabVEKy Code_yKemxoFi3p

Desktop

rodolphocastro commented 3 years ago

Did some digging around and found the source for asserts: https://github.com/drashland/rhum/blob/37b56a4e5f9dfd667711cd3a776371a3d34c89ad/src/rhum_asserts.ts#L4

My guess is this could be some issue related to exporting as a destructuring from deps.ts which actually holds a full import as * from std/asserts?

ebebbington commented 3 years ago

Thanks @rodolphocastro, we'll look into it - i assume it is the way we are declaring asserts (or assigning the vlaues to it) because it's pulling everything from std/testing/asserts, and as you can see, we are doing something like:

// deps.ts
export * as asserts from "https://deno.land/std@0.70.0/testing/asserts.ts";

// rhum.ts
import { asserts as StdAsserts } from "deps.ts"
const asserts = { ...StdAsserts };

Btw really good descriptive issue, the screennshots are super helpful too! :) 🎉

rodolphocastro commented 3 years ago

Just noticed that even inside rhum_asserts.ts the auto-complete goes hay-wire 😅

image

This suck as it might require some duplication to actually get it working... I've done a fork and I'll try my luck at this if you don't mind haha, worst case scenario we'd end up with some "mapping" from std/asserts and it'd require some manual updates over the time (altho I'm not sure how frequently the asserts over @ std would change for this to become cumbersome hahaha)

ebebbington commented 3 years ago

Feel free to take a dig at it :) The idea is that Rhum.asserts is just every assert method from std

Maybe even asserts = StdAsserts might provide the intellisense, but then again, i don't use vscode nor any extensions that help :P

rodolphocastro commented 3 years ago

No such luck with const something = StdAsserts as well :/

Seems that once we're relying on that import/export once we export it ourselves we lose the metadata and it just becomes any to vscode.

Is it working as it should on your IDE? If so it could be an issue on the deno extension itself and not with Rhum haha

ebebbington commented 3 years ago

I get no intellisense support (i'm using PHPStorm with no plugins/extensions) :P Makes sense what you're saying - though it might be good to bring it up with the extension developers on the discord (or I can ask if you'd prefer), because it might be an extension issue? Regardless the issue can still be worked on

rodolphocastro commented 3 years ago

If you could bring it up I'd appreciate it! It's almost 11pm and I'm about to head off to sleep haha.

Did a last shot at the destructuring approach and nada yet image image image

I'm against code duplication but if this ends up not being an issue on the extension itself it might be worth, from a module consumer's point of view, to have that autocompletion even if it means some manual mapping... What do you think?

ebebbington commented 3 years ago

Yeah alright i'll have to give it a try tomorrow though (3am atm), but it's not ideal but i'm not against it, will see what @Guergeiro @crookse @saragee3 think

rodolphocastro commented 3 years ago

@ebebbington Actually the change might be needed just on the mod.ts itself

Just noticed that the asserts get a new typing once they get up to mod.ts and after removing that typing and allowing ts to infer it things worked like a charm 🎊

image image

(Note: this does make 2 integration tests fail so it's def. not final hahah)

I'll commit my changes as WIP on the fork for now. Maybe applying a public-only interface to RhumRunner.asserts that maps the "official" asserts would be enough 💡

Commit with changes so far: https://github.com/rodolphocastro/rhum/commit/f23b8c9c9735e40c643bfc62f466f0852a663e0f

Edit: Failing output

error: TS2775 [ERROR]: Assertions require every name in the call target to be declared with an explicit type annotation.
      Rhum.asserts.assert(true);
      ~~~~~~~~~~~~~~~~~~~
    at file:///E:/Src/rhum/tests/integration/asserts_test.ts:6:7

    'Rhum' needs an explicit type annotation.
    export const Rhum = new RhumRunner();
                 ~~~~
        at file:///E:/Src/rhum/mod.ts:470:14

TS2775 [ERROR]: Assertions require every name in the call target to be declared with an explicit type annotation.
    Rhum.asserts.assert(functionWasCalled);
    ~~~~~~~~~~~~~~~~~~~
    at file:///E:/Src/rhum/tests/unit/mod_test.ts:60:5

    'Rhum' needs an explicit type annotation.
    export const Rhum = new RhumRunner();
                 ~~~~
        at file:///E:/Src/rhum/mod.ts:470:14

Found 2 errors.
rodolphocastro commented 3 years ago

I might have a solution!

Using an extra type that looks into assert keys + some changes to the existing typing that was in-use before my commit:

image image

As a bonus this one doesn't break any of the integration tests 👍 https://github.com/rodolphocastro/rhum/commit/a794bd040b324f709bc10d8ccb289e6204577f03

But it doesn't help with the parameters anymore 🤦‍♂️ image

ebebbington commented 3 years ago

@rodolphocastro looking into it now

ebebbington commented 3 years ago

@rodolphocastro come to the discord as it might be easier: https://discord.gg/NMe97z

i'm not even getting any intellisense after typing Rhum :S

ebebbington commented 3 years ago

Cracked the badger

ebebbington commented 3 years ago

image

and we can still do `const asserts = { ... StdAsserts }

rodolphocastro commented 3 years ago

Sweet! If you still need I'll be able to hop into discord in a few hours but seems everything is golden now 💪

ebebbington commented 3 years ago

Seems like it, and you can be the one to mark sure it works when we release ;) My PR pretty much just used what you have done in this issue, so most credit goes to you 👍

I would never have thought of doing key in denoAssertions

rodolphocastro commented 3 years ago

Me neither hahah. That idea came to me as I was getting ready to sleep and I thought "what if we used the keyof function?". Then I quickly turned on my laptop and took that shot from the previous comment 😂

Once that gets published I'll be sure to come by and mark this as working 🥳