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

Documentation update: Fixes issue #1557 #1662

Merged
merged 85 commits into from Sep 12, 2014
Merged

Documentation update: Fixes issue #1557 #1662

merged 85 commits into from Sep 12, 2014

Conversation

@MarkGriffiths
Copy link
Contributor

@MarkGriffiths MarkGriffiths commented Sep 4, 2014

  • Moves entire documentation to Markdown format. Much simpler.
  • Fully supports Doxygen 1.8.7+
  • All documentation targets updated: user_doc, share/man, doc and doc/refman.pdf.
  • Tested across Ubuntu, CentOS and Mac OS.

Output preview:

MarkGriffiths added 30 commits Aug 1, 2014
Rework for Doxygen >1.8. Moved large parts of the documentation to a
simplified format, making use of Markdown enhancements and fixing bad
long options.
Subllime and a few temporary files.
Conflicts:
	doc_src/history.txt
	doc_src/test.txt
Addresses issue #1557 as well as fixing many typos, HTML errors and
inconsistencies. Also introduces automatic syntax colouring and enables
new documentation to be written in Markdown. TODO fix Tutorial.
 Fixed manpage 'NAME'. Under Doxygen 1.8, the output format has
changed, so the old sed script was leaving man pages with two titles.
Conflicts:
	doc_src/index.hdr.in -- UPDATED
	doc_src/license.hdr -- UPDATED
This reverts accidentally included files in commit
60b9f8d.
This rolls back to commit 60b9f8d.
If the lexicon input filter isn't specified (as is the case in the
current Xcode project, the script quietly continues without it.
Completely fixes #1557 and the underlying Doxygen changes that caused
it. Should make fish docs simpler and more robust, more consistent and
generally prettier.

todo:
- trap unmarked text as arguments in context
- test & fix sed portability - see in particular. (so far tested on BSD
(Mac) and GNU sed).
- test Makefile changes
- last round of aesthetic changes and getting that ascii fish in there…
Conflicts (FIXED):
	.gitignore
	doc_src/complete.txt
	doc_src/function.txt
Conflicts (FIXED):
	doc_src/command.txt
	doc_src/index.hdr.in
	doc_src/read.txt
	doc_src/type.txt
@MarkGriffiths
Copy link
Contributor Author

@MarkGriffiths MarkGriffiths commented Sep 7, 2014

Done, and preview links above updated.

Full rationale/style guide in user_doc/FORMATTING.md (also posted in issue #1557)

@xfix xfix added this to the next-minor milestone Sep 7, 2014
@xfix
Copy link
Member

@xfix xfix commented Sep 7, 2014

Approved 👍.

Normally, I would merge a commit after approving it. However, this is a big change (24,380 new lines is huge, even if there is a good reason for that), so I would prefer to wait with merging for other fish shell contributors to approve this particular pull request.

@MarkGriffiths
Copy link
Contributor Author

@MarkGriffiths MarkGriffiths commented Sep 7, 2014

Totally understandable. It took that amount of editing to catch everything in doc_src - mind you, just the three updated Doxyfiles account for ~6000 lines of changes!

@lilyball
Copy link
Member

@lilyball lilyball commented Sep 7, 2014

14425 of the added lines are a brand new Portuguese translation file, which apparently comes from the merge of master into this branch.

@lilyball
Copy link
Member

@lilyball lilyball commented Sep 8, 2014

More generally, git diff --stat says that, compared to current master, this is 9951 insertions, 5732 deletions.

Makefile.in Outdated
user_doc: $(HDR_FILES_SRC) Doxyfile.user $(HTML_SRC) $(HELP_SRC) doc.h $(HDR_FILES) lexicon_filter
(cat Doxyfile.user; echo INPUT_FILTER=./lexicon_filter; \
echo PROJECT_NUMBER=$(FISH_BUILD_VERSION) | sed "s/-.*//") | doxygen - && touch user_doc; \
cd user_doc/html; rm -f bc_s.png bdwn.png closed.png ftv2*.png nav*.png open.png sync_*.png tab*.* doxygen.* dynsections.js jquery.js pages.html

This comment has been minimized.

@lilyball

lilyball Sep 8, 2014
Member

This should probably be cd user_doc/html && rm -f ..., no? That way if the user_doc folder fails to be created in the first place, this won't attempt to delete files in the current directory.

Makefile.in Outdated
cd doc/latex; \
make; \
mv refman.pdf ..; \
cd ../..; \

This comment has been minimized.

@lilyball

lilyball Sep 8, 2014
Member

Presumably this should be

    cd doc/latex && \
    make && \
    mv refman.pdf ..

i.e. don't run commands if previous ones failed. And the trailing cd ../.. is unnecessary if you simply don't escape the newline.

Makefile.in Outdated
WORDBL='[[:<:]]'; WORDBR='[[:>:]]'; \
else\
WORDBL='\\<'; WORDBR='\\>'; \
fi; \

This comment has been minimized.

@lilyball

lilyball Sep 8, 2014
Member

I think this whole if statement is wrong. Not only is the conditional bad, but the else branch is also wrong.

For the conditional, you're testing the exit code of sed. But you're also treating the output as if it were a command, which is inappropriate. In the event that the sed returns 0, but doesn't delete the line, it will then attempt to execute the command x. Instead, to just test the exit code, you probably want

    if echo x | sed "/[[:<:]]x/d" >/dev/null 2>&1; then \

That properly tests the status code without attempting to interpret the output as a command. It also suppresses any output just in case. Ideally we'd also test to make sure it output nothing, but I think that's a little overkill.

As for the else conditional, you're setting the vars to e.g. '\\<'. The extra backslash is unnecessary. /bin/sh does not interpret backslashes inside single-quotes. If you echo the variable it will, but only because the echo builtin interprets escapes. However, you're not passing the results to echo, you're passing them to sed, and sed will not want the extra backslash.

@lilyball
Copy link
Member

@lilyball lilyball commented Sep 8, 2014

Aside from the couple of Makefile.in changes that need to be done, :shipit:.

Conflicts (resolved):
	doc_src/design.hdr - \c changed to backticks
@lilyball
Copy link
Member

@lilyball lilyball commented Sep 9, 2014

At this point I'm inclined to merge it, the only thing stopping me from hitting the switch right now is the fact that Doxygen 1.8.7 is not available by default on Ubuntu 14.04 and earlier, and likely other Linux distros as well. I'm not sure what process is used to build the nightlies or full releases, and whatever that process is, it needs to be able to build the docs.

@ridiculousfish or someone else who knows, is the Doxygen 1.8.7 requirement an issue or is this good to merge?

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 9, 2014

I wouldn't mind the chance to test it because I suspect the nightly builds will stop generating documentation.

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 12, 2014

OK, this definitely doesn't support 1.8.1.2 (which is in Debian Stable) - $extrastylesheet doesn't get expanded so there's no styling. I don't mind backporting on the machine which builds the nightlies, but I wonder whether someone could try 1.8.6 which the latest release of Ubuntu has, as does Fedora 20.

@MarkGriffiths
Copy link
Contributor Author

@MarkGriffiths MarkGriffiths commented Sep 12, 2014

I tried 1.8.6 under Ubuntu and found that Doxygen is missing the new \htmlonly[block] directive which came in under 1.8.7 (the version that homebrew installed when I began working on this). The result of this was the word 'block' appearing at the top of each semantic block, i.e. the top of each page and above \fish blocks.

\htmlonly[block] is important for the docs as it keeps the block elements in the HTML output in the structural order expressed in the source (similar to using \parblock, but smarter) and it fixed many HTML structural issues that were present in the old documentation. Personally, I've been waiting for this feature for a long time!

In my own testing on Ubuntu and CentOS I had to build Doxygen (1.8.8) from GitHub source. I was hoping that as fish ships with prebuilt documentation that this wouldn't have been too much of an issue.

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 12, 2014

It certainly does ship with prebuilt documentation; my concern is the raising of the barrier for contributing to the documentation. However there are prebuilt binaries available on the Doxygen website so I think it would be reasonable to expect people to be able to download them. I think Markdown lowers the bar for contributing and I am very grateful for that! Let's merge.

xfix added a commit that referenced this pull request Sep 12, 2014
Documentation update: Fixes issue #1557
@xfix xfix merged commit 5c25be5 into fish-shell:master Sep 12, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@davidxia
Copy link
Contributor

@davidxia davidxia commented Sep 15, 2014

\o/ This is awesome!

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Sep 16, 2014

Holy cow!

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 17, 2014

Yikes. I just noticed the license on some of the files. Would you consider relicensing lexicon_filter either under the GPL2 like the rest of fish, or a permissive BSD-style license? "whatever terms are most compatible with Fish's GPLv2 license" is not clear. We should also add it to doc_src/license.hdr.

@MarkGriffiths
Copy link
Contributor Author

@MarkGriffiths MarkGriffiths commented Sep 17, 2014

I don't consider it a serious or formal license in any way at all - it's simply intended as a tongue-in-cheek way of saying "blimey, I've spent far too much time, missed too much sleep and drunk too much coffee getting this right than would be considered sane, yet it's my gift to the fish community so I'd better say something...". My intention is that it should fall under fish's license completely and unreservedly. Apologies if that didn't come across, I simply hoped it would raise a smile, and in 20+ years of publishing software and digital media I've never had to enforce a license... British humour I guess 😉

I'm happy for that to be completely stripped out or modified in any way to make it clear.

That said, I felt it should say something as the filter's mechanism is fairly novel and self-contained and demonstrates how to create custom syntax colouring in Doxygen using an input filter - something often asked for in the Doxygen community, but rarely answered. It would be a useful template for others to start from.

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 18, 2014

That would be great. Would you forgive me for being the Fun Police and raise a PR with that change? It just needs to say "this code is licensed with fish under the GPL 2.0" but I would be happier if the commit had your authorship on it!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants