Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Cleanups#3014

Merged
cdecker merged 5 commits intoElementsProject:masterElementsProject/lightning:masterfrom
cdecker:cleanupscdecker/lightning:cleanupsCopy head branch name to clipboard
Sep 2, 2019
Merged

Cleanups#3014
cdecker merged 5 commits intoElementsProject:masterElementsProject/lightning:masterfrom
cdecker:cleanupscdecker/lightning:cleanupsCopy head branch name to clipboard

Conversation

@cdecker
Copy link
Member

@cdecker cdecker commented Aug 30, 2019

Just some cleanups regarding the python tests.

Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

I'm not sure if you encounter this error too ? For example :

rm doc/lightning-autocleaninvoice.7
make doc-all


$(MANPAGES): doc/%: doc/%.md
@if $(CHANGED_FROM_GIT); then echo mrkd $<; mrkd $<; else touch $@; fi
if $(CHANGED_FROM_GIT); then mrkd $< $@; else touch $@; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

You should either mrkd $< -output $@ or remove the $@ otherwise you got a mrkd: error: unrecognized arguments: [the_manpage_name.manpage_number] when running make doc-all. It's weird that travis doesn't fail on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we may have different versions of mrkd. My help shows the following:

$ mrkd --help
usage: mrkd [-h] [-name NAME] [-section SECTION] [-template TEMPLATE]
            [-index INDEX] [-format roff] [-colors friendly]
            source output

positional arguments:
  source              The source man page
  output              The output file

optional arguments:
  -h, --help          show this help message and exit
  -name NAME          The name to use for the man page
  -section SECTION    The section to use for the man page
  -template TEMPLATE  The HTML template file to use
  -index INDEX        An index file to use for HTML links
  -format roff        The output format
  -colors friendly    The Pygments style to use for HTML syntax highlighting

Notice that output is a positional argument, not a flag argument. This is btw also the mrkd that is deployed on travis, and is the one installed via pip install mrkd

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the tweaked version introduced in #2936. I think you had problem because you used the original version : actually I think most of the modification I made in the tweaked version (https://github.com/darosior/mrkd/commits/master) are necessary to have a clean output. That's why I added this version in doc/requirements.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

This is btw also the mrkd that is deployed on travis

Ah I thought Travis would install the one in doc/requirements.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I had not realized that you customized the mrkd utility.

All but the last commit seem to be also addressed by 14f4e3c. What's the purpose of that last commit? And would it be ok if we used the pypi version, in order to have less custom code?

Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw I found LIGHTNING-CHECK(7) Manual Page (for the <h1/> title and the html <title/>) way too shouty and not really informative. With the switch to what formerly was the description we now get an <h1/> and <title/> similar to this lightning-fundchannel – Command for establishing a lightning channel which I find less shouty and more descriptive 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I actually looked at the only diff which had a duplicated description x)

command-subcommand - Short description was more of a title anyway imho

I agree

Copy link
Contributor

@darosior darosior Sep 1, 2019

Choose a reason for hiding this comment

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

darosior/mrkd@dfbc8c3 has been dropped on my branch, so doc/requirements.txt needs to be updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw I gave you write access to the repo, since it is used as a dependency here.

Copy link
Contributor

Choose a reason for hiding this comment

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

darosior/mrkd@dfbc8c3 has been dropped on my branch, so doc/requirements.txt needs to be updated

What about adding a tag?

@cdecker
Copy link
Member Author

cdecker commented Sep 1, 2019

I'm not sure if you encounter this error too ? For example :

rm doc/lightning-autocleaninvoice.7
make doc-all

That's exactly the reason for the first commit :-)

=========================================
lightning-autocleaninvoice -- Set up auto-delete of expired invoice
===================================================================
lightning-autocleaninvoice - Set up auto-delete of expired invoice
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing description :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch, good catch.

@darosior
Copy link
Contributor

darosior commented Sep 2, 2019

Tagged mrkd and opened cdecker#4 which fixes Travis errors.

cdecker and others added 5 commits September 2, 2019 13:37
mrkd started enforcing the `name -- short description` style of top-level
headings somewhere, and was thus failing to build the man-pages. I swapped
the title and with the existing short description to make it work
again. `mrkd` will automatically infer the section from the filename so no
need to put it in the title as well.

In addition I removed the "last updated" lines at the bottom since they are
out of date at best, and misleading at the worst. If we want to keep them, I'd
suggest generating them from the commit that last touched them.
This is an issue that was raised in ElementsProject#2665: some of the dependencies where
causing warnings to be added to the logs about deprecated dependencies. Since
I did not get these warnings I just blanket updated all the dependencies in
the hopes of getting the warnings to resolve.

Signed-off-by: Christian Decker <@cdecker>
We were checking the test request against the searched for string. This fixes
it by actually looking at the outcome instead and should clean up correctly
if tests do not fail.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
We're not going to optimize travis any longer so let's no fill logs with
useless measurements.
Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK b31ea82

@cdecker cdecker merged commit 0e2ec27 into ElementsProject:master Sep 2, 2019
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.