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 #247

Merged
merged 2 commits into from
Jul 26, 2021
Merged

Misc doc changes #247

merged 2 commits into from
Jul 26, 2021

Conversation

kianmeng
Copy link
Contributor

Besides other documentation changes, this commit ensures the generated
HTML doc for HexDocs.pm will become the source of truth for this Elixir
library and leverage on latest features of ExDoc.

@cpjolicoeur
Copy link
Member

@kianmeng Thanks for the pull request. I have a few questions and change requests though.

First, I'm not sure what you mean by "this commit ensures the generated
HTML doc for HexDocs.pm will become the source of truth for this Elixir
library". This is already the case, is it not? We generated updated documentation
and upload it every time we release a new version of the library. I'm not sure how these changes ensures that in a way beyond what we already have and do?

Second, you made some additional changes to other files that seem unrelated to the description. For example, some changes to the .formatter.exs file, .gitignore, changing the LICENSE file to markdown. Things like that are unrelated and we'd want to drop them from the commit and this pull request before merging.

Also, some of the formatting changes made to the README file itself are unneeded. I'm not very interested in adding badges for the code coverage amount, or the last commit information. Some indentation and whitespace changes within the README content as well are not necessary as well.

README.md Outdated

Copyright (c) 2016 Infinite Red, Inc.

This software is released under the [MIT License](./LICENSE.md).
Copy link
Member

Choose a reason for hiding this comment

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

This whole added section is not needed (somewhat incorrect) and can/should be removed.

Copy link
Contributor Author

@kianmeng kianmeng Jul 25, 2021

Choose a reason for hiding this comment

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

You're right about the incorrectness, it supposed to be:

Original work Copyright (c) 2016 Infinite Red, Inc. 
Modifed work Copyright (c) 2019 MojoTech, LLC. 

I've removed it as requested.

elixir: "~> 1.8",
start_permanent: Mix.env() == :prod,
compilers: [:phoenix, :gettext] ++ Mix.compilers(),
name: "Torch",
description: "Rapid admin generator for Phoenix",
source_url: "https://github.com/mojotech/torch",
homepage_url: "https://github.com/mojotech/torch",
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this line for homepage_url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Added it back.

Besides other documentation changes, this commit ensures the generated
HTML doc for HexDocs.pm will become the source of truth for this Elixir
library and leverage on latest features of ExDoc.
@kianmeng
Copy link
Contributor Author

@kianmeng Thanks for the pull request. I have a few questions and change requests though.

@cpjolicoeur Thanks for the feedback and letting me know your preference.

First, I'm not sure what you mean by "this commit ensures the generated
HTML doc for HexDocs.pm will become the source of truth for this Elixir
library". This is already the case, is it not? We generated updated documentation
and upload it every time we release a new version of the library. I'm not sure how these changes ensures that in a way beyond what we already have and do?

Basically all related documents in the repository like CHANGELOG.md, CODE_OF_CONDUCT.md, and LICENSE can be accessed through HexDocs.

Second, you made some additional changes to other files that seem unrelated to the description. For example, some changes to the .formatter.exs file, .gitignore, changing the LICENSE file to markdown. Things like that are unrelated and we'd want to drop them from the commit and this pull request before merging.

Reverted these changes. LICENSE was originally renamed to LICENSE.md for consistency.

Also, some of the formatting changes made to the README file itself are unneeded. I'm not very interested in adding badges for the code coverage amount, or the last commit information. Some indentation and whitespace changes within the README content as well are not necessary as well.

Likewise, changes reverted.

@kianmeng kianmeng requested a review from cpjolicoeur July 25, 2021 14:56
@cpjolicoeur
Copy link
Member

Thanks @kianmeng for this Pull Request.

I made some small changes to your README updates to make sure keys were set in the proper sections in the mix file.

@cpjolicoeur cpjolicoeur merged commit 396892a into mojotech:master Jul 26, 2021
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.

2 participants