Skip to content

Replace keyword with concrete keyword lists in specs #14611

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

Merged
merged 3 commits into from
Jun 30, 2025

Conversation

lukaszsamson
Copy link
Contributor

This PR:

  • Replaces keyword/term with concrete keyword lists in specs for all functions taking options argument
  • Adds missing documentation for some of the supported options
  • Adds specs for functions with options argument

Rationale:

  • Better LSP completions. ElixirLS is able to parse specs and provide completions for keyword options
  • Dialyzer validation
  • Easier machine transformation to elixir type annotation
  • Better consistency. A big part of the API surface already had typed keyword lists.

I originally planned to do this only for public functions, but I realized that it would be better to do it for all functions taking options argument. Some of the internal ones were already documented or had specs.

…s taking options argument

Add missing documentation for some of the supported options
Add specs for functions with options argumnet
@@ -128,6 +128,37 @@ defmodule IO do
@type nodata :: {:error, term} | :eof
@type chardata :: String.t() | maybe_improper_list(char | chardata, String.t() | [])

@type inspect_opts :: [
Copy link
Member

Choose a reason for hiding this comment

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

Should we reuse Inspect.Opts.new_opts here? If not, let's make sure to remove the deprecated options too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Beautiful, just one tiny comment and we are good to go!

@josevalim josevalim closed this Jun 30, 2025
@josevalim josevalim reopened this Jun 30, 2025
@josevalim
Copy link
Member

CI is failing, can you please take a look? Please rebase if necessary. :)

@josevalim josevalim merged commit e3f7759 into elixir-lang:main Jun 30, 2025
12 of 13 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Copy link
Contributor

@michallepicki michallepicki left a comment

Choose a reason for hiding this comment

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

@@ -58,7 +65,7 @@ defmodule IEx.Broker do
@doc """
Client requests a takeover.
"""
@spec take_over(binary, iodata, keyword) ::
@spec take_over(binary, iodata, take_over_opts) ::
Copy link
Contributor

Choose a reason for hiding this comment

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

This spec needs to cover options passed in from lib/iex/lib/iex/pry.ex:40 , so binding, dot_iex, env and stacktrace

"""
@spec to_quoted(term(), to_quoted_opts) :: String.t()
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy-paste mistake, this should be a spec for to_quoted_string

[{Path.t(), Path.t()}],
atom(),
atom(),
compile_opts,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to support opts passed from compile.yecc.ex and compile.leex.ex, I think all_warnings and verbose are currently missing

@josevalim
Copy link
Member

Thank you, I am pushing some fixes. A couple of those changes are also pass through and we were limiting the interface. I already had some WIP locally, I will include your feedback as I wrap up.

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.

3 participants