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 IEx tests on OTP27: fix assertions #13352

Closed
wants to merge 1 commit into from

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Feb 16, 2024

Opening this PR for discussion.

We migrated IEx introspection to use :shell_docs in #12803, but OTP27 stops returning specs, which would be a regression for us.

Before (type)

Screenshot 2024-02-16 at 13 43 22

After (type)

Screenshot 2024-02-16 at 13 43 41

(The opaque information got lost as well, not great)

Before (spec)

Screenshot 2024-02-16 at 13 45 35

After (spec)

Screenshot 2024-02-16 at 13 45 58

It might be fine given we're aiming at replacing typespecs with set-theoretic types, but I wanted to confirm.
Or should we open an issue upstream?

@lukaszsamson
Copy link
Contributor

You can fall back to reading specs via Code.Typespec

@sabiwara
Copy link
Contributor Author

You can fall back to reading specs via Code.Typespec

This would basically imply reverting #12803 and go back to generating it ourselves rather than relying on :shell_docs though right? This is what I wanted to confirm.

@josevalim
Copy link
Member

Btw, OTP 27 has markdown docs. So maybe we should use our own rendering structure for Erlang if the content type is markdown?

@garazdawi
Copy link

Another alternative is to wait for erlang/otp#8077 to be merged, then you can continue to use shell_docs for rendering erlang docs.

@josevalim
Copy link
Member

Good point. So maybe it is best to wait until the next release candidate is out and then we can assess all of our options more throughly? @sabiwara

@sabiwara
Copy link
Contributor Author

Good point. So maybe it is best to wait until the next release candidate is out and then we can assess all of our options more throughly?

Sounds good!
Thank you for the quick reply @garazdawi !

@sabiwara sabiwara closed this Feb 16, 2024
@sabiwara sabiwara deleted the otp27-iex-introspection branch February 16, 2024 09:34
@sabiwara
Copy link
Contributor Author

I think this will be the last blocker to add OTP27 to the CI, once the other PRs are merged.

@josevalim
Copy link
Member

@sabiwara we can also stop asserting for specs on tests and add an issue to introduce the specs back once 27 is out (and such an option exists). Your call. :)

@sabiwara
Copy link
Contributor Author

sabiwara commented May 17, 2024

Hi @garazdawi,

Another alternative is to wait for erlang/otp#8077 to be merged, then you can continue to use shell_docs for rendering erlang docs.

I was trying the master branch after 8077 has been merged, but it seems that shell_docs are not displaying specs:

master:
Screenshot 2024-05-17 at 17 07 38

OTP26:
Screenshot 2024-05-17 at 17 07 47

I wondered if specs are planned to be added back to :shell_docs in OTP27, or if I misunderstood.
Thank you for your support!

@garazdawi
Copy link

Yes, it is planned though it won’t make it to 27.0 as a bunch of other things popped up that needed to be done. We would be happy to accept a pr if you feel like helping out!

@sabiwara
Copy link
Contributor Author

sabiwara commented May 17, 2024

We would be happy to accept a pr if you feel like helping out!

I would love to! Will give it a shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants