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

Fix #587 #603

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Fix #587 #603

merged 3 commits into from
Feb 26, 2021

Conversation

jonludlam
Copy link
Member

The error was due to a bug in the shadowing code, which manifests due to Subst's recent tightening in the PR to improve speed.

Signed-off-by: Jon Ludlam <[email protected]>
This still isn't quite right, but it's definitely less wrong with this
fix.

Signed-off-by: Jon Ludlam <[email protected]>
if List.mem name map.shadowed.s_types then
`Type
( parent,
Odoc_model.Names.TypeName.internal_of_string (Ident.Name.type_ id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Odoc_model.Names.TypeName.internal_of_string (Ident.Name.type_ id)
Odoc_model.Names.TypeName.internal_of_string name

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good catch :-)

@@ -0,0 +1,19 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#!/bin/sh
#!/usr/bin/env sh

Otherwise, this script can be inlined into run.t.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it quite helpful to have the script external as it's easier to run and find the output - particularly for those that output html that I'd like to look at (not this one, as it happens). Is there a better way to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My preferred way is to add commands to the run.t file, I usually add $ find . then cat some files. I do dune runtest --auto-promote test/xref2 to increase speed (I have a binding in my editor for that).
That way I'm sure everything is run from scratch and I avoid inconsistent states.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, that's fine for just seeing the content of the files but I often want to see the HTML output in the browser. I suppose that's really only when developing the test, so I can always transcribe it into the run.t afterwards, as I agree it's better to have it inline.

@jonludlam jonludlam merged commit d6f68bf into ocaml:master Feb 26, 2021
@jonludlam jonludlam mentioned this pull request Mar 3, 2021
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.

2 participants