evently-cloud / cli

Evently Command-Line Interface
Apache License 2.0
1 stars 0 forks source link

Conversion to Ketting #9

Closed evert closed 1 year ago

evert commented 1 year ago

@mattbishop I was gonna adopt eslint from ketting with these settings:

https://github.com/badgateway/ketting/blob/main/.eslintrc.json

One thing is that they do include and auto-fix semicolons, but I can change the settings to not do that. Let me know if you're down with eslint and if you prefer to take all semi-colons out. I slightly prefer them but definitely not enough to argue about it =)

evert commented 1 year ago

Forgot to fix the unittests, will take some more time because they rely on Undici's mocking support (which is neat!) but this uses node-fetch under the hood and there's no equivalent =(

mattbishop commented 1 year ago

I like eslint, as long as we can burn the semicolons with fire! I spent many years writing C and Java, so it reminds me of where I am to not use semicolons.

On Wed, Mar 15, 2023 at 4:57 PM Evert Pot @.***> wrote:

@mattbishop https://github.com/mattbishop I was gonna adopt eslint from ketting with these settings:

https://github.com/badgateway/ketting/blob/main/.eslintrc.json

One thing is that they do include and auto-fix semicolons, but I can change the settings to not do that. Let me know if you're down with eslint and if you prefer to take all semi-colons out. I slightly prefer them but definitely not enough to argue about it =)

— Reply to this email directly, view it on GitHub https://github.com/evently-cloud/cli/pull/9#issuecomment-1470998688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACY3BLTFSWMXOHUYQG5BRDW4JJNJANCNFSM6AAAAAAV4JQRQU . You are receiving this because you were mentioned.Message ID: @.***>

evert commented 1 year ago

I like eslint, as long as we can burn the semicolons with fire! I spent many years writing C and Java, so it reminds me of where I am to not use semicolons.

Alright! Will take some getting used to =)

evert commented 1 year ago

I'll do that in a later PR so this one doesn't get too noisy.

evert commented 1 year ago

Hey @mattbishop ,

I tried to fix the unittests but ran into all kinds of strange issues with types. I suspect that this is because partially the codebase thinks it's working with the new built-in Request and Response classes from fetch, and partially those from node-fetch.

I haven't really had issues with this in the past, because they are similar enough. One area they distinctly aren't is how they do streams, which is where all my issues are.

So one idea I have is to stop using node-fetch. This is possible if I release Ketting 8 as a beta. I've already made this change here, but I wanted to run this by you in case you believe this may constitute scope creep.

Pretty frustrating TBH! You can see also how I've tried to implement mocking functionality with the ledger:download test. Not super happy with the verbosity there so I thought it might also be helpful there to add some utilities that make it easy to see if an incoming HTTP request roughly matches a pattern.

LMK, so far I've spent about 6 hours on the entire project.

evert commented 1 year ago

@mattbishop ready for a deeper review, I did all the above and everything now works the way I wanted.

I will follow up with another PR that removes all the semicolons (permanently)