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

Misc doc changes #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kianmeng
Copy link

@kianmeng kianmeng commented Feb 3, 2021

List of changes:

  • Fix spdx license id
  • Add source reference
  • Fix markdown
  • Use and set latest ex_doc
  • Add license section
  • Badges and more badges!
  • Update gitignore
  • Revise documentation

@safwank
Copy link
Owner

safwank commented Feb 8, 2021

Terima kasih 😄. LGTM. The badges are a nice touch!

I'll merge it some time this weekend 👍🏽.

README.md Outdated
def application do
[applications: [:retry]]
end
def application do

Choose a reason for hiding this comment

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

This whole section should be removed. Retry doesn't have an application

Copy link
Author

Choose a reason for hiding this comment

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

This whole section should be removed. Retry doesn't have an application

You're right, it's already inferred. Thanks.

:applications - all applications your application depends on at runtime. By default, this list is automatically inferred from your dependencies. Mix and other tools use the application list in order to start your dependencies before starting the application itself.

Choose a reason for hiding this comment

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

No, it's not inferred, there is nothing in this library that calls use Application which means that there is no runtime Application in the OTP sense. This library doesn't provide an Application it just provides a set of macros and functions.

Copy link
Author

Choose a reason for hiding this comment

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

I see. Does this means the :retry app from Application.started_applications/0 have different meaning?

iex -S mix
iex> Application.started_applications() |> Enum.filter(fn {app, _, _} -> app == :retry end)
[
  {:retry,
   [32, 32, 83, 105, 109, 112, 108, 101, 32, 69, 108, 105, 120, 105, 114, 32,
    109, 97, 99, 114, 111, 115, 32, 102, 111, 114, 32, 108, 105, 110, 101, 97,
    114, 32, 114, 101, 116, 114, 121, 44, 32, 101, 120, 112, 111, 110, 101, 110,
    116, 105, 97, 108, 32, 98, 97, 99, 107, 111, 102, 102, 44, 32, 97, 110, 100,
    32, 119, 97, 105, 116, 10, 32, 32, 119, 105, 116, 104, 32, 99, 111, 109,
    112, 111, 115, 97, 98, 108, 101, 32, 100, 101, 108, 97, 121, 115, 46, 10],
   [48, 46, 49, 52, 46, 49]}
]

Choose a reason for hiding this comment

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

You know, I don't know. I thought I did but I poked around a bit today and I'm stumped! Perhaps we get a default one provided for us. Regardless, Retry doesn't have any supervisors or other processes that need to be started when the client app boots, so I think it's still safe to remove the instruction there.

Copy link
Author

Choose a reason for hiding this comment

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

Just FYI.

I digged around and found this is related to application inference, which was introduced since Elixir 1.4.

Since this is a legacy Elixir module developed since Elixir 0.13.1 (2014), the installation guide for Elixir 1.3 or earlier, as you mentioned, should be removed since the minimum supported version right now is Elixir 1.8.

I've also updated the application/0:

 35   def application do                                                                                    
 36     [                                                                                                   
 37       extra_applications: [:logger]                                                                     
 38     ]                                                                                                   
 39   end

Copy link
Owner

Choose a reason for hiding this comment

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

The reason why I left [applications: [:retry]] in the README is because from experience it's still required when running an app built using distillery or release, even if it's just a library. Without it, you'll get a runtime error saying the module is missing or some such.

Happy to be proven wrong.

Copy link
Author

Choose a reason for hiding this comment

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

The reason why I left [applications: [:retry]] in the README is because from experience it's still required when running an app built using distillery or release, even if it's just a library. Without it, you'll get a runtime error saying the module is missing or some such.

Happy to be proven wrong.

@safwank Sorry, I've missed this message. Based on your reply, we should add back the [applications: [:retry]] back in the README then?

List of changes:
- Fix spdx license id
- Add source reference
- Fix markdown
- Use and set latest ex_doc
- Add license section
- Badges and more badges!
- Update gitignore
- Revise documentation
- Move logger app to extra_applications
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.

4 participants