Skip to content

Merge benchmark container to parent repository #117

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
Merged
merged 48 commits into from
Oct 16, 2023
Merged

Merge benchmark container to parent repository #117

merged 48 commits into from
Oct 16, 2023

Conversation

erlingdevold
Copy link
Contributor

image
image
image

markhilb
markhilb requested changes Oct 13, 2023
View reviewed changes
Copy link
Member

@markhilb markhilb left a comment

Choose a reason for hiding this comment

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

Still some errors in the console.
I added another action that checks for these.
If you push again it will run so you can see the errors.

@VetleWoie
Copy link
Contributor

Seemed to be a problem related to this typescript issue: microsoft/TypeScript#44063 (comment)

Managed to fix it by changing the logic slightly.

markhilb
markhilb requested changes Oct 13, 2023
View reviewed changes
markhilb
markhilb requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@markhilb markhilb left a comment

Choose a reason for hiding this comment

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

Left some more comments and unresolved some.

FYI, don't resolve comments when you make changes.
It is the reviewer who resolves them when they agree the comment has been resolved

fhf-datafangst-client/src/store/benchmark/actions.ts
import * as Api from "api";
import { BenchmarkDataSource, BenchmarkModalParams } from "./state";

export interface benchmarkHistoricParams {
Copy link
Member

Choose a reason for hiding this comment

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

Types/interfaces should be TitleCase

markhilb
markhilb reviewed Oct 16, 2023
View reviewed changes
@erlingdevold erlingdevold requested a review from markhilb October 16, 2023 10:57
@markhilb markhilb merged commit b3da43e into orcalabs:master Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VetleWoie VetleWoie VetleWoie left review comments

@markhilb markhilb markhilb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erlingdevold @VetleWoie @markhilb