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

Add step-logs.js to crunch. #799

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Conversation

novemberkilo
Copy link
Contributor

@novemberkilo novemberkilo commented Apr 3, 2023

This PR is a second attempt at bringing the work in #794 to production. In addition to re-introducing the changes from #794 it deals with the issue of not including the newly generated javascript file to ocaml-crunch.

Also, when trying to build a docker image the deployer failed to find a recent version of jsoo so I've updated the opam-repository commit sha. The latest opam-repository includes the alpha5 release of Dream so I've dropped the pins on Dream entirely.

Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks!

// Called via jsoo in the generated step-logs.js
function extractStepsToReproduce(startIndex, finishIndex) {
if ( // return if already done
!!document.getElementById("build-repro-container") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if the double bang is necessary, isn't the value already truthy/falsy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The double bang is "idiomatic" js for Boolean(object)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but the values you're testing are already in a "boolean context", the if condition, so they're already evaluated as booleans using their thruthy/falsy semantics. To my understanding, you only need the double bang when storing the "boolean value" of a value, or comparing the value to another using "boolean" semantics.

Comment on lines +209 to +211
match Astring.String.cuts ~sep:"/" (List.hd refs) with
| "refs" :: "heads" :: branch -> Astring.String.concat ~sep:"/" branch
| _ -> ""
Copy link
Contributor

@MisterDA MisterDA Apr 3, 2023

Choose a reason for hiding this comment

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

Maybe check that "refs/heads/" is a prefix of List.hd refs instead.


let collapse_carriage_returns log_line =
let rec last = function
| [] -> raise (Failure "Trying to take log_line from empty list (BUG)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| [] -> raise (Failure "Trying to take log_line from empty list (BUG)")
| [] -> Fmt.failwith "Trying to take log_line from empty list (BUG)"

web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
Comment on lines +1 to +5
module Ev = Brr.Ev
module El = Brr.El
module At = Brr.At
module Document = Brr.Document
module Window = Brr.Window
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module Ev = Brr.Ev
module El = Brr.El
module At = Brr.At
module Document = Brr.Document
module Window = Brr.Window

Can't this be remove?

ocaml-ci-web.opam Outdated Show resolved Hide resolved
dune-project Outdated Show resolved Hide resolved
@novemberkilo novemberkilo merged commit cf3930c into ocurrent:master Apr 4, 2023
@novemberkilo novemberkilo deleted the fix/ws-logs branch April 4, 2023 01:21
novemberkilo added a commit that referenced this pull request Apr 5, 2023
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.

4 participants