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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Type hint support #93

Merged
merged 4 commits into from
Jun 5, 2024
Merged

Type hint support #93

merged 4 commits into from
Jun 5, 2024

Conversation

Kukovec
Copy link
Collaborator

@Kukovec Kukovec commented Jun 4, 2024

closes #92

Extends fetch with a --typemap flag, with which users may submit type hints (described in more detail in fetch.ts), which are used in instrument to distinguish vectors from enums, when v.every((elem) => typeof elem === typeof v[0] is an insufficient filter condition.

Note that all changes to the .json files are semantically trivial (a new field with type hits is added)

thpani
thpani reviewed Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

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

Looks great!

I'm not sure if the mustHaveHint (for vec) is comprehensive.

Otherwise, just some suggestions.

solarkraft/src/fetch.ts Outdated Show resolved Hide resolved
solarkraft/src/fetch.ts Outdated Show resolved Hide resolved
solarkraft/src/fetch.ts Outdated Show resolved Hide resolved
solarkraft/src/fetch.ts Show resolved Hide resolved
solarkraft/src/fetcher/callDecoder.ts Outdated Show resolved Hide resolved
solarkraft/src/fetcher/callDecoder.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
@thpani
Copy link
Collaborator

thpani commented Jun 5, 2024

Ah also, afaict there's no tests?

The type hints in the e2e are empty.

Kukovec
Kukovec commented Jun 5, 2024
View reviewed changes
Copy link
Collaborator Author

@Kukovec Kukovec left a comment

Choose a reason for hiding this comment

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

RE tests, yeah, I had planned to add them later, just cause I finished this batch of commits later in the day than I'd estimated. Would you want them as part of this PR or a followup?

solarkraft/src/fetch.ts Outdated Show resolved Hide resolved
solarkraft/src/fetch.ts Outdated Show resolved Hide resolved
solarkraft/src/fetcher/callDecoder.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
solarkraft/src/instrument.ts Outdated Show resolved Hide resolved
@Kukovec Kukovec requested a review from thpani June 5, 2024 12:13
thpani
thpani approved these changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

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

LGTM, but please follow-up with tests

@Kukovec Kukovec merged commit 417ec2e into main Jun 5, 2024
3 checks passed
@Kukovec Kukovec deleted the jk/typeHints branch June 5, 2024 13:05
@konnov konnov added this to the M2: Monitor executor milestone Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thpani thpani thpani approved these changes

@konnov konnov Awaiting requested review from konnov

@andrey-kuprianov andrey-kuprianov Awaiting requested review from andrey-kuprianov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
M2: Monitor executor
Development

Successfully merging this pull request may close these issues.

Add type hint support for Enums
3 participants
@Kukovec @thpani @konnov