Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linting #367

Merged
merged 8 commits into from
Nov 22, 2024
Merged

Linting #367

merged 8 commits into from
Nov 22, 2024

Conversation

mrsimpson
Copy link
Collaborator

@mrsimpson mrsimpson commented Nov 21, 2024

What's in here

This PR adds linting to the fork. Since the current codebase contains a lot of linting issues, this takes the smalles possible step to ensure code-consistency.

Motivation

Linting surfaces code smells. Linting with the same rules ensures that PR diffs are kept small and readable.

How it's been done

As per the commits, I did the following

  • Lint-fix what can be fixed easily
  • ignore some uncritical rules
  • fix other errors

Verdict

While checking the linting, there were quite some code-smells like inconsistent returns of function/methods. I tried to fix them so that they are syntactically fine. I couldn't always judge whether this is desirable wrt the intended architecture.

I manually tested the application (prompted, edited files and so on), lgtm. But it would be great if somebody threw another pair of eyes on it 👀

Don't know why, but previously the linter for . didn't terminate, although node_modules is ignored as per linter config
@coleam00
Copy link
Collaborator

Thank you for getting started on this @mrsimpson, I appreciate it a lot!

@mrsimpson mrsimpson changed the title [WIP] Linting Linting Nov 22, 2024
@wonderwhy-er
Copy link
Collaborator

hm, small merge caused conflict, at some point we need to sync and merge this

@wonderwhy-er wonderwhy-er self-requested a review November 22, 2024 16:13
@mrsimpson
Copy link
Collaborator Author

@wonderwhy-er

This is what I meant when I wrote ;)

Also, the overall mechanism of loading new LLMs is very … simple and relies on a couple of files being added. Usually, I’d expect some registry-mechanism here to allow for modification free extensibility when e. g. adding new providers.

Small changes to the providers cause conflicts (even semantically compatible ones ).

I merged the main branch again, so here's another chance to get linting fixed ;)

@wonderwhy-er
Copy link
Collaborator

Running checks

Sadly as merge is possible without running checks we may still get unlinted things in

@mrsimpson mrsimpson mentioned this pull request Nov 22, 2024
@wonderwhy-er
Copy link
Collaborator

CI/CD failed with typecheck

@mrsimpson
Copy link
Collaborator Author

Sadly as merge is possible without running checks we may still get unlinted things in

I added #378 to prevent this

@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Nov 22, 2024

CI/CD failed with typecheck yes, saw this.

I introduced a type where things were not ... clear.
I'll update this PR and check the types again. Once I'm done with this, I'll notify you here, @wonderwhy-er .
I'd add the typecheck to #378 afterwards ;)

I'll have to take a break for some hours now though – other commitments :(

@mrsimpson
Copy link
Collaborator Author

@wonderwhy-er sort-of fixed the type checker: The reason why the type check on the previous commit failed was that it was ironically cleaner than the main branch.

In some files, Stackblitz hast put a @ts-nocheck in order to make the code look more ... mature on Youtube. When I linted it, //@ts-nocheck was converted into a block comment due to the subsequent line – and was not recognized by the tsc as excempted anymore.
And as there was no forced tsc (even build doesn't trigger it), I missed this 🙈

Now, in order to keep this PR as low-invasive as possible, I reverted those affected files in which I had started to introduce better types to the main branches state, including the ts-nocheck. This is actually worse, but we'll tackle this later.

All quality checks pass on my end now.
image

Kindly verify and merge before new conflicts appear.
You may also consider merging #378 in order to make things worse again soon.

@wonderwhy-er wonderwhy-er merged commit 7fc8e40 into stackblitz-labs:main Nov 22, 2024
1 check passed
@wonderwhy-er
Copy link
Collaborator

Merged

@coleam00
Copy link
Collaborator

Thanks @wonderwhy-er and @mrsimpson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants