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

feat: show fortune #128

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: show fortune #128

wants to merge 12 commits into from

Conversation

mnishiguchi
Copy link
Member

@mnishiguchi mnishiguchi commented Jun 15, 2023

revised after developing this separate project called elixir-fortune

Description

resolves #105

  • This feature is powered by the elixir-fortune package (support include/exclude options fhunleth/elixir-fortune#1)
  • By default, this feature is disabled
  • With the fortune: true option specified, one of the default Nerves tips will be printed in the MOTD
  • Users can optionally provide custom fortunes in their Elixir projects, and include fortunes provided by dependencies, following the elixir-fortune instructions

CleanShot 2023-09-07 at 22 47 16--1

Notes

  • The default tips are copied from https://tips.nerves-project.org
  • The general nerves tips could alternatively live in other projects since elixir-fortune is capable of scanning all the dependencies for fortunes directories at the time users compile their Nerves projects.

TODO

  • release elixir-fortune v0.1.0 first
  • update fortune dependency (currently path dependency)
  • make sure all tests pass
  • merge this PR
  • release new version of nerves_motd

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from b5f5420 to 547905a Compare June 15, 2023 02:06
@mnishiguchi mnishiguchi marked this pull request as ready for review June 15, 2023 02:16
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from 547905a to 8d975ea Compare June 15, 2023 13:16
@jjcarstens
Copy link
Member

We chatted about this at standup and are super excited! Some of the proposed changes:

  • Shift to one-line tips
    • Store in a file in this repo for now
  • Allow users to specify custom tips and/or files as well

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch 2 times, most recently from bfab933 to a5826fd Compare July 19, 2023 23:36
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from be74a25 to 94436e7 Compare September 8, 2023 01:59
@mnishiguchi mnishiguchi changed the title feat: show tip of the day feat: show fortune Sep 8, 2023
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch 2 times, most recently from f61e155 to 85db7e1 Compare September 8, 2023 13:02
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from 85db7e1 to 4fb4c2d Compare November 10, 2023 13:31
@mnishiguchi
Copy link
Member Author

mnishiguchi commented Nov 10, 2023

I did some testing locally and it worked well.

Three parts

  • elixir-fortune
    • defines default elixir fortune file
    • provides :strfile_compiler compiler
    • provides Fortune.random!(options)
      @typedoc """
        The fortune options
      
        * `:paths` - a list of absolute paths to `fortunes` directories
        * `:include` - a list of applications whose fortunes you want to opt in for
        * `:exclude` - a list of applications whose fortunes you want to opt out of
        """
        @type fortune_option() ::
                {:paths, String.t() | [String.t()] | nil}
                | {:include, atom | [atom] | nil}
                | {:exclude, atom | [atom] | nil}
  • nerves_motd
    • depends on elixir-fortune
    • uses :strfile_compiler compiler
    • defines nerves-tips fortune file
      # mix.exs
      compilers: Mix.compilers() ++ [:strfile_compiler]
  • user’s Nerves firmware project
    • depends on nerves_motd
    • defines custom fortune file
      # rootfs_overlay/etc/iex.exs
      NervesMOTD.print(fortune: true)

Tip format

  • wrapping fortune entries by following rules in fortune files is working well
    • width: 80-ish
    • height: 1-5 lines

Inclusion / exclusion of fortune files

Can be configured in a user project like below.

config :fortune, include: [...]
config :fortune, exclude: [...]
config :fortune, paths: [...]

@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from 5bd6f76 to 88c5041 Compare November 30, 2023 00:44
@fhunleth
Copy link
Member

This is looking good.

I think we should start moving to reduce the NervesMOTD code for tips. That way, anyone who doesn't want them won't feel like they're bloating their firmware. I suspect that the reality is that this strings only trivially affect it, but I do know a few people who would prefer to not even have to look.

How about:

  1. Start using a separate library for the tips. e.g., :nerves_tips
  2. Make the :fortune dependency optional.
  3. Make a :nerves_tips dependency just for dev and test. For prod, I don't think we need a dependency on it at all, but I can see how it would be handy for dev and test.
  4. Keep the option to show the fortune. I'd propose renaming it to :show_fortune? to make it look more boolean. If it's unset, make it default to true if the :fortune application is available at runtime and :false if not. That way it should just work for most users and they won't need to concern themselves with the configuration.

WDYT?

config/config.exs Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from b6c7c1a to db62ff6 Compare December 5, 2023 00:45
Co-authored-by: Frank Hunleth <[email protected]>
@mnishiguchi mnishiguchi force-pushed the mnishiguchi/tip-of-the-day branch from db62ff6 to bf4871b Compare December 5, 2023 00:46
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.

Support showing a tip
3 participants