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

[gh-265] Add the IEx addon #266

Merged
merged 8 commits into from
Sep 27, 2022
Merged

[gh-265] Add the IEx addon #266

merged 8 commits into from
Sep 27, 2022

Conversation

byhbt
Copy link
Member

@byhbt byhbt commented Sep 20, 2022

Closes #265

What happened 👀

  • Add the IEx addon to generate the default .iex.exs add the project root directory
  • Add elixir_ls to the generated .gitignore
  • Remove the RFC template in the Issue section. From now on, to propose a new RFC, we will use GitHub Discussion instead.

Insight 📝

The WebApp.Repo is only loaded for the web project.

Proof Of Work 📹

image

image

The tests should pass.

@byhbt byhbt self-assigned this Sep 20, 2022
@byhbt byhbt force-pushed the chore/add-default-iex-exs-file branch from a45f8eb to 43c935b Compare September 20, 2022 04:07
@byhbt byhbt added this to the 4.4.0 milestone Sep 20, 2022
@byhbt byhbt marked this pull request as ready for review September 20, 2022 08:08
@byhbt byhbt requested a review from a user September 20, 2022 08:08
Copy link
Member

@liamstevens111 liamstevens111 left a comment

Choose a reason for hiding this comment

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

Few questions otherwise I think it's a good addition to the template 👍

Comment on lines 19 to 20
string: :light_black,
boolean: :red,
Copy link
Member

Choose a reason for hiding this comment

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

These are the only changes that are not default, right? I think we should leave the colors to be the default as they are subjective unless there's a benefit for doing so. I do like differentiating between the colors of booleans and nil though as they are both magenta in the default colours?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the default output:

image

Compare with the customized output:

image

As you can see that the colors have improved for readability. So as a developer, you can get started with that and don't take more time in finding colors to customization.

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion, it's good to keep the color as default if nothing wrong with it. I removed the override the code colors here: @liamstevens111 0d37bf5

If the developer would like to override the syntax colors, they can override it in ~/.iex.exs.

Copy link
Member

Choose a reason for hiding this comment

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

@byhbt So if we confirmed the colours in your terminal (all the same) are not the default colours and they actually have distinct colours as previously mentioned (although bools and nil are same 😢 ) then yeah I think should be left as default IMO. 👍

alias <%= base_module %>.Repo
<% end %>
IEx.configure(
history_size: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

This history_size is rather large 😀 , what is the default? I couldn't find it online and it seems whatever I change this value to it doesn't really have any effect.

Copy link
Member Author

@byhbt byhbt Sep 26, 2022

Choose a reason for hiding this comment

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

Here is the default of history_size https://github.com/elixir-lang/elixir/blob/main/lib/iex/mix.exs#L19 . Yeah admitted that it's a bit larger than the default one. I will update it to just ten times larger seem enough. Updated in 960406e

Copy link
Member

@liamstevens111 liamstevens111 Sep 26, 2022

Choose a reason for hiding this comment

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

I see, you need to have history enabled first right, ie

iex --erl “-kernel shell_history enabled”
https://hexdocs.pm/iex/1.14/IEx.html#module-shell-history

I guess if this was enabled above I could just press up to scroll back my history instead of typing v(n) to go back, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we need to enable it on the system as a whole.

export ERL_AFLAGS="-kernel shell_history enabled"

Copy link
Member

@liamstevens111 liamstevens111 left a comment

Choose a reason for hiding this comment

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

Just small comment about history otherwise good to go

#266 (comment)

I see, you need to have history enabled first right, ie

iex --erl “-kernel shell_history enabled” https://hexdocs.pm/iex/1.14/IEx.html#module-shell-history

I guess if this was enabled above I could just press up to scroll back my history instead of typing v(n) to go back, right?

@byhbt byhbt merged commit f99fa8d into develop Sep 27, 2022
@byhbt byhbt deleted the chore/add-default-iex-exs-file branch September 27, 2022 02:39
@byhbt byhbt mentioned this pull request Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chore] Add default .iex.exs file
6 participants