Skip to content

Fix (some) of the unspecified tpyes #42

Draft
OnionKiller wants to merge 12 commits intomainfrom
propagate-types
Draft

Fix (some) of the unspecified tpyes #42
OnionKiller wants to merge 12 commits intomainfrom
propagate-types

Conversation

@OnionKiller
Copy link
Copy Markdown
Member

@OnionKiller OnionKiller commented Aug 11, 2023

In the name of strongly typed languages, and thus a better development experience, I set out to improve the type safety of the project. This is in fact probably impossible, and the scope should be better defined, but....
This far, there are significant desing decisions, which makes typing really hard, and require some convoluted type magic. This is also maybe considered by some as a fun activity. Also maybe helps with the <G:any,ctx:any, Promise<any,any>> extends any and similar situations which may lead to missing fileds, or broken usages of objects.
⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⡏⠠⢉⠒⣄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢰⡊⠉⠒⠲⠤⣄⠀⠀⠀⠀⠀⠀⠀⠀⠉⣹⢸⢳⡈⢢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⠈⣏⠲⡀⢷⡀⠀⠀⠀⠀⠀⣀⠜⢁⡾⠀⢹⡌⡇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠀⢀⡀⠀⠀⠀⠀⠀⢷⠸⡆⠹⣄⢳⡀⢀⠤⠒⣋⡤⠖⠋⠀⠀⡼⢃⡇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⢸⡁⠈⠑⢄⡀⠀⢀⠼⢁⡗⠀⢸⡄⣷⠉⡴⠋⠁⠀⠀⠀⢠⠞⣠⡞⠙⢆⠀⠀⠀⠀⢀⣤⢤⡀⠀⠀⠀⠀⠀⠀⠀
⠀⠀⠀⠀⠀⠀⠀⠀⠙⣆⢰⣄⢳⡼⢁⡴⠋⠀⠀⢸⡇⠛⣸⠁⠀⠀⠀⠀⠀⡏⣴⣿⠇⣰⡄⢳⡀⠀⢀⠾⢠⢀⡧⠀⠀⠀⠀⠀⠀⠀
⠀⠀⢀⣤⠤⣄⡀⠀⣠⠏⡸⢹⡎⠁⡾⠀⠀⠀⠀⠸⢬⠷⠽⠀⣠⠀⠀⠀⠀⠻⡀⡤⣰⠃⠹⢤⣉⡟⢁⡴⣿⠉⣍⠳⡀⠀⠀⠀⠀⠀
⠀⠀⠈⢷⡀⢀⠸⠾⢁⡼⠁⠸⡇⢸⡇⠀⠀⠀⠀⠀⠀⠀⠀⠰⠋⠀⠀⠀⠀⠀⠈⠉⠁⠀⠀⢸⠃⢠⡏⠀⢿⣀⣿⢦⠘⡆⠀⠀⠀⠀
⠀⠀⠀⠀⣳⠄⣷⢠⡎⠀⠀⠀⠙⢦⣷⠀⢸⠄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⢧⣀⣷⠀⠀⠈⠁⢨⡇⣽⠀⠀⠀⠀
⠀⠀⠀⢠⠇⣼⢹⣸⡇⠀⠀⠀⠀⠀⠀⢠⡇⠀⠀⠀⠀⠀⠀⣀⣠⣤⣀⠀⢀⣀⣀⣀⠀⠀⠀⠀⠀⠀⠸⠀⠀⣠⣶⣉⣴⣿⣷⣀⣀⣤
⠀⢿⣷⣾⣶⣷⣿⣏⠀⠀⠀⠀⠀⠀⠀⠈⠀⠀⠀⠀⠀⢠⡾⠉⣷⣆⡬⢷⡋⣿⠧⠉⢳⡄⠀⠀⠀⠀⠀⠀⠀⢻⣿⣿⣿⣿⣿⣿⣿⣍
⢶⣿⣿⣿⣿⣿⣿⡏⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣤⣾⣇⠀⠈⣾⠀⠀⢹⡆⠀⠀⢀⣿⣤⡀⠀⠀⠀⠀⠀⣠⣿⣿⣿⣿⣿⣿⠛⠛
⠀⣿⣿⣿⣿⣿⣿⣿⣦⡀⠀⠀⠀⠀⠀⠀⠀⢀⣴⣿⣿⣿⣿⣿⣿⣿⡀⢀⣼⣿⣷⣾⣿⣿⣿⣿⣷⣀⡀⢀⣼⣿⣿⣿⣿⣿⡏⠈⠲⠀
⠈⠛⢻⡙⣿⣿⣿⣿⣿⣿⣄⠀⠀⠀⢴⣶⣶⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣏⣾⣿⣿⣿⣿⣿⡿⠃⠀⢀⠀
⠀⠀⡼⠧⡈⠻⣿⣿⣿⣿⣿⣷⣄⠀⠀⢹⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⡿⠛⠒⠀⢀⡼⠃
⢀⣠⡇⢠⣌⣡⠼⠻⣿⣿⣿⣿⣿⣧⡀⠿⠿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⡿⠋⠀⠀⠀⠀⢩⠄⡏
⠸⡄⢧⣄⡙⢦⡀⠀⠘⠻⣿⣿⣿⣿⣿⣷⣦⣾⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠋⠀⠀⠀⢀⣀⡤⠞⣰⠏
⠀⠳⡈⠳⣌⠉⠀⠀⠀⠀⠀⠹⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠃⠀⠀⠀⠀⣼⠀⠶⡾⠁⠀
⠀⠀⠈⠢⣌⣳⡄⠀⠀⢢⡀⠀⠈⠻⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⡟⠀⠀⠀⠀⠀⠙⢲⡆⡇⠀⠀
⠀⠀⠀⠀⠊⡹⠃⠀⠀⠀⠃⠀⠀⠀⣹⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⠀⠀⠀⣠⠤⠚⢉⡤⠃⠀⠀

@OnionKiller OnionKiller added the enhancement New feature or request label Aug 11, 2023
@OnionKiller OnionKiller self-assigned this Aug 11, 2023
for instantiation
█▀▄▀█ ▄▀█ █▀▄ █▀▀   █▀ █▀█ █▀▄▀█ █▀▀   █▀▄▀█ ▄▀█ █▀▀ █ █▀▀  
█░▀░█ █▀█ █▄▀ ██▄   ▄█ █▄█ █░▀░█ ██▄   █░▀░█ █▀█ █▄█ █ █▄▄  
ALSO DOENST WORKS yet
LET THE HAIL BEGIN
@OnionKiller OnionKiller linked an issue Aug 11, 2023 that may be closed by this pull request
into the huge composit type
it turns out the bot class have a very different enumerate function from teh general ai
enumerate function, which caused discrepancies in the types
also sidenote, NonNullable is the type time equivalent of !. type operator,
so there are som forced type casts which may lead to unexpected result. I don't really
care about that  right now, as this part is hopefully not critical now
Fuck my life. Also TODO: fix this to be more sensible.
Also TODO: figure out why it doesn't works
This reverts commit 17a1218.
@radl97
Copy link
Copy Markdown
Contributor

radl97 commented Aug 11, 2023

I am afraid to check the code.

How is it doing? You haven't marked it draft before. Is the code ready for merge?

Nit: Also, instead of revert, you might find fixup, autosquash and force pushing rhe feature branch a better candidate.

but also great use of commot msg conventions

//this actually results in a somewhat correct type, altough it doesn't implements some of the private methods, for some reason ts doesn't complains about that
class _Bot extends Bot {
// waits 400 ms for UX
// waits 400 ms for UX, and is a private type
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do not understand this comment. You wanted to add this to the class header maybe?

Also, I think wait coul be marked private

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be.
This comment was more like to keep track of what's going on.

@OnionKiller OnionKiller marked this pull request as draft August 13, 2023 11:15
@OnionKiller
Copy link
Copy Markdown
Member Author

OnionKiller commented Aug 13, 2023

I'm still working on it, because it doesn't solves all the issues.
Also there seems to be an issue with how the type imports in boardgame are implemented, buuut it's inconsistent. After npm ci there are parts of the koa types which are not properly included, and thus unresolved. But if I modify teh src code of the types.d.ts then it starts to work, even if I revert the changes. I don't properly understand the root of this. I guess VS code shinenigans.
Edit: its definetly vs code related, but they use a different import strategy rather than the self explained, but it seems to be fine.
(they use import type Koa from koa instead of import type * as Koa from koa, but it seems to be fine)

@radl97
Copy link
Copy Markdown
Contributor

radl97 commented Aug 13, 2023

Feel free to merge partial results.

(they use import type Koa from koa instead of import type * as Koa from koa, but it seems to be fine)

I think they are not the same thing:

export A, export B, export default C

if you import * from koa, A, B, C will be available in the global scope: C.c();

If you import * from koa as Koa, the Koa object will contain properties A, B, C. Koa.C.c(); I think.

However, if you import Koa, you will have a single object, which is C. Koa.c();... I think...

@radl97
Copy link
Copy Markdown
Contributor

radl97 commented Aug 13, 2023

Feel free to merge.

by which I mean:

If you have a partial result, which works, and it is not hacky, then it should be merged, I think. So that the branch always contains the best version of the code, in quality, typing, etc.

If it is too hacky, it might be best to improve it before merging.

The npm ci should be the one that works: if the CI job works, it should not be a problem to replicate a successful build.

@OnionKiller
Copy link
Copy Markdown
Member Author

OnionKiller commented Aug 14, 2023

Feel free to merge partial results.

(they use import type Koa from koa instead of import type * as Koa from koa, but it seems to be fine)

I think they are not the same thing:

export A, export B, export default C

if you import * from koa, A, B, C will be available in the global scope: C.c();

If you import * from koa as Koa, the Koa object will contain properties A, B, C. Koa.C.c(); I think.

However, if you import Koa, you will have a single object, which is C. Koa.c();... I think...

You are right, but inside the Koa type definition they have a big umbrella export calss, and internally everything is collected into one namespace. There is only one export inside Koa or Koa-routing as matter of fact, so it has now difference in this case. It is porbably not the best way to do it like this, but i'm not sure if I'll make the pull request for this.
This difference also seems to confuse VS code sometimes, but thats just a transient error with typing in an IDE, not an actual issue.

@OnionKiller
Copy link
Copy Markdown
Member Author

OnionKiller commented Aug 14, 2023

If it is too hacky, it might be best to improve it before merging.

It's kinda hacky right now, but i'm starting to figure stuff out. It will be significantly better soon, and the main parts will be ok-ish especially from type support point of game development, which is the main concern really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TODO solve typing

2 participants