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

Enhance installation docs and add release download URL #1656

Merged
merged 1 commit into from Oct 10, 2018

Conversation

dnsmichi
Copy link
Contributor

fixes #1648

Copy link
Contributor

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, sorry for the delay. Reason for not merging it immediately: unrelated formatting changes. The "how to download the module" part equals other modules and is fine.

Regarding the formatting: we try to keep Markdown as readable as possible in your preferred text editor. It's not for web readers only, otherwise we would ship HTML. In case you have good reasons to change parts of it, suggestions are always welcome. They should then be applied to all files equally and handled in a dedicated pull request.

Thanks,
Thomas

@dnsmichi
Copy link
Contributor Author

My intention with changing the formatting is to later rewrite the entire documentation, following the style we intend to keep with Icinga 2, Icinga Web 2 and modules. It isn't necessarily part of this PR, you're correct here. I'll split this up into a separate PR.

@dnsmichi dnsmichi force-pushed the feature/enhance-installation-docs branch from 3719642 to ebb664f Compare October 10, 2018 07:19
@dnsmichi
Copy link
Contributor Author

@Thomas-Gelf I've extracted the parts relevant for the download URL and methods keeping the current formatting, rebased and force pushed this PR. Please merge :)

@Thomas-Gelf
Copy link
Contributor

@dnsmichi: before spending a lot of time for reformatting everything please let's eventually discuss the motivation behind that first. There isn't much in this pull request, but for a better understanding please let me address the changes I saw and the objections I have:

  • adding id's everywhere: this is ugly and makes it hard to read in your text editor. We're currently forced to do so mainly for the main title and for titles with special characters. Both are shortcomings in our documentation parser and should sooner or later be addressed there. Final goal should be to not be required to manually fiddle with IDs at all. It's ugly and error-prone. As of today, titles which do not fall into the mentioned categories should require no ID at all
  • changing main titles from:
Header 1
========

Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam
erat, sed diam voluptua. At vero eos et accusam et justo duo dolores
et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est
Lorem ipsum dolor sit amet.


Subtitle
--------
Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam
erat, sed diam voluptua. At vero eos et accusam et justo duo dolores
et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est
Lorem ipsum dolor sit amet.

...to...

# Header 1

Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam
erat, sed diam voluptua. At vero eos et accusam et justo duo dolores
et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est
Lorem ipsum dolor sit amet.


## Subtitle

Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam
erat, sed diam voluptua. At vero eos et accusam et justo duo dolores
et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est
Lorem ipsum dolor sit amet.

I (personally) consider the first example being more readable in text editors, especially when there is a lot of text. People used to markdown might be fine with the second example, non-tech people might find it easier to scan the document structure with the first one.

  • preformatted text / code: we always add the GitHub-flavored language/dialect suffix when there is formatted code, like SQL, PHP and so on. We use indentation (4 spaces) where we have just preformatted text blocks, like short installation instructions. Once again, they are easier to scan in your text editor.

We shouldn't get too much into details here, by the end it's not related to this issue at all. I just wanted to explain my point of view on the formerly proposed changes. I'm sure there will be other opinions, so let's find some spare time to address them.

Cheers,
Thomas

@Thomas-Gelf Thomas-Gelf merged commit 0349a10 into master Oct 10, 2018
@Thomas-Gelf
Copy link
Contributor

Merged, thanks @dnsmichi!

@Thomas-Gelf Thomas-Gelf deleted the feature/enhance-installation-docs branch October 10, 2018 07:30
@Thomas-Gelf Thomas-Gelf added this to the 1.6.0 milestone Oct 10, 2018
@dnsmichi
Copy link
Contributor Author

One thing about the <a id... markers are readable URLs where you can link between documents - GitHub renders them slightly different from the headers than MKdocs, and after all this is really tricky from my experience.
In terms of the general format - I think we need to sit down and agree on a specified format somewhere in the future.

For now, I'm fine with following the current format and partially adding new content :)

Cheers,
Michi

Thomas-Gelf added a commit that referenced this pull request Dec 10, 2018
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.

Git Hub link is not available in Installation Page
2 participants