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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add more type definitions #882

Merged
merged 3 commits into from Oct 27, 2022
Merged

Add more type definitions #882

merged 3 commits into from Oct 27, 2022

Conversation

FabioRosado
Copy link
Contributor

I started adding some types in an effort to fix #834 but Jeff discovered the issue and fixed it 馃槃

This PR adds a bit of typing across the board - I'm still learning how to use the more advanced features of typescript, so there are some changes that I am unsure of 馃

Also I'm not entirely sure if we should replace some/all of these any for unknown

@antocuni
Copy link
Contributor

ouch, sorry to stumble upon your feet, but note that in PR #881 I'm doing (yet another 馃槄) heavy refactoring which among the other things kills a lot of code, include some which was also touched by you.
In particular, the PyScript class becomes very thin, andBaseEvalElement is used only as a base class for PyRepl.
It might make more sense to wait for that PR to land and then rebase... sorry :(

@FabioRosado
Copy link
Contributor Author

ouch, sorry to stumble upon your feet, but note that in PR #881 I'm doing (yet another sweat_smile) heavy refactoring which among the other things kills a lot of code, include some which was also touched by you. In particular, the PyScript class becomes very thin, andBaseEvalElement is used only as a base class for PyRepl. It might make more sense to wait for that PR to land and then rebase... sorry :(

Haha no worries! I'll wait for that PR to be in and rebase on it 馃槃

marimeireles
marimeireles approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Types! 馃ぉ

@FabioRosado
Copy link
Contributor Author

I'm waiting for #884 to get in before I rebase 馃槃

@antocuni
Copy link
Contributor

I'm waiting for #884 to get in before I rebase smile

yes, after #884 I don't have any major refactoring planned :)

@fpliger
Copy link
Contributor

fpliger commented Oct 25, 2022

Nice PR! Thanks @FabioRosado

yes, after #884 I don't have any major refactoring planned :)

.... yet 馃槅

@madhur-tandon
Copy link
Member

@FabioRosado this should be rebased now since #884 is merged now 馃槄

madhur-tandon
madhur-tandon requested changes Oct 27, 2022
View reviewed changes
Copy link
Member

@madhur-tandon madhur-tandon left a comment

Choose a reason for hiding this comment

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

Rebase required 馃槄

madhur-tandon
madhur-tandon approved these changes Oct 27, 2022
View reviewed changes
Copy link
Member

@madhur-tandon madhur-tandon left a comment

Choose a reason for hiding this comment

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

@FabioRosado Yours to merge when the tests pass :)

@FabioRosado
Copy link
Contributor Author

Thanks @madhur-tandon I can't merge it though 馃槃

Just ran npm run lint and noticed some more types missing, I'll probably will open a follow up PR with them once I'm done unless this PR is still open then I will push here

@madhur-tandon
Copy link
Member

I'll probably will open a follow up PR with them once I'm done unless this PR is still open then I will push here

Whenever you are ready 馃挴

@fpliger
Copy link
Contributor

fpliger commented Oct 27, 2022

Alt Text

FabioRosado
FabioRosado commented Oct 27, 2022
View reviewed changes
pyscriptjs/src/components/pytitle.ts
@@ -1,4 +1,4 @@
import { addClasses, htmlDecode, ensureUniqueId } from '../utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antocuni just checking, should pytitle call ensureUniqueId as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that py-title should die in flames 馃槄.
Looking at the code I think it should because it's indeed using this.id.
But on the other hand, I don't think that the id is used for anything useful, so we could just avoid using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha removing things is always fun!

Awesome I just wanted to be sure that I didn't removed something that we would use in the future 馃榾

@FabioRosado
Copy link
Contributor Author

There are still about 71 warnings but a lot of them are coming from pyodide since most of the methods are typed upstream as any, but I think for now this is good 馃榿

@tedpatrick tedpatrick merged commit 1c53d91 into pyscript:main Oct 27, 2022
@tedpatrick
Copy link
Contributor

Awesome work

@FabioRosado FabioRosado deleted the fr/types branch October 28, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antocuni antocuni antocuni left review comments

@marimeireles marimeireles marimeireles approved these changes

@madhur-tandon madhur-tandon madhur-tandon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PyScript OSS
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pre-commit CI is failing
6 participants
@FabioRosado @antocuni @fpliger @madhur-tandon @tedpatrick @marimeireles