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

Filter out Fold markers. #3

Merged
merged 5 commits into from
Jun 7, 2016
Merged

Conversation

ypid
Copy link
Member

@ypid ypid commented May 28, 2016

It started out when I introduced more and more fold markers to the DebOps project. I used ((( as marker because it was not highlighted by my editor as being a Jinja2 control sequence. The fold marker as turned out to be not-optimal because it also appears in complex Jinja2 conditions quite often. So @drybjed and me settled on [[[. The problem was, however, that we always needed to introduce additional redundancy to the files. Example:

---
# .. vim: foldmarker=[[[,]]]:foldmethod=marker

# Default variables
# =================

# .. contents:: Sections
#    :local:
#
# .. Role configuration [[[1
#
# ----------------------
#   Role configuration
# ----------------------

@drybjed and I discussed this and agreed that the best thing would be to patch up yaml2rst to filter out the fold marker which would allow it to be directly appended after headings for instance. Example:

---
# .. vim: foldmarker=[[[,]]]:foldmethod=marker

# Default variables
# =================

# .. contents:: Sections
#    :local:
#
# ----------------------
#   Role configuration [[[1
# ----------------------

With this patch, this input gets converted into:

.. vim: foldmarker=[[[,]]]:foldmethod=marker

Default variables
=================

.. contents:: Sections
   :local:

----------------------
  Role configuration
----------------------

Can we make this the default behavior or should it be made optional? I limited the regex to only match a very defined set of occurrences.

ypid added a commit to ypid/ansible-packages that referenced this pull request May 28, 2016
ypid added a commit to ypid/ansible-preseed that referenced this pull request May 28, 2016
debops/yaml2rst#3 needs to be released before
the docs build test will pass.
@ypid
Copy link
Member Author

ypid commented May 28, 2016

I just realized that using folds this way is handicapped because you can not easily move around folded sections. The problem is that the upper part of the RST section is not part of the fold anymore. Moving section around is one of the main advantages of folds. I would therefore propose to change the heading format to:

# .. contents:: Sections
#    :local:
#
# Required packages [[[
# -----------------

# .. envvar:: x2go_server__base_packages
#
# List of base packages to install.
# You can checkout `ypid.packages <https://github.com/ypid/ansible-packages/>`_
# which can install additional packages to support your desktop environment
# better.
x2go_server__base_packages:
  - 'x2go-keyring'
  - 'x2goserver'
  - 'x2goserver-xsession'
# .. ]]]

# Software sources [[[
# ----------------
#

This keeps folds fully functional without the need to just stuff after a move.

ypid added a commit to ypid/ansible-packages that referenced this pull request May 29, 2016
ypid added a commit to ypid/ansible-packages that referenced this pull request May 29, 2016
Never change a running system!
Related to debops/yaml2rst#3 (comment)
@drybjed
Copy link
Member

drybjed commented May 29, 2016

I've just looked at the reStructuedText primer and it says that section titles that are just underlined are different from those that use underline and overline, and are placed on different levels I assume that means that if we switch to the underline-only sections, this will break the documentation at http://docs.debops.org/, unless we convert everything at once.

In that case, for the time being I'm fine with staying with the current formatting requirements that add vim folds, ie.:

# .. Section title [[[1

# -----------------
#   Section title
# -----------------

I wonder if there is an alternative to RST that could be more suited for this, but probably not. I'll try to find out if Ansible team has any plans for role documentation.

@htgoebel
Copy link
Collaborator

I suggest filtering these marks only if a command-line option tells so. I'd suggest this option to also take the string to be taken as fold-marks, e.g. --strip-foldmarks=[[[,]]]

@@ -101,6 +101,9 @@ def convert(lines):
elif line.startswith('# ') or line == '#':
if state != STATE_TEXT:
yield ''
# Filter out [[[\d and ]]] at the end of a documentation line.
# Those sequences can be used for folding sections.
line = re.sub(r"\s*(:?\[{3}|\]{3})\d?$", "", line)
Copy link
Collaborator

@htgoebel htgoebel May 29, 2016

Choose a reason for hiding this comment

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

  • This would strip foldmarks in every line, not only in rst-comments
  • \s* is of no use?
  • What should the trailing digit be? (Please document)

Copy link
Member Author

@ypid ypid Jun 2, 2016

Choose a reason for hiding this comment

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

This would strip foldmarks in every line, not only in rst-comments

It should only affect text lines. Can you double check?

\s* is of no use?

It will additionally strip any trailing whitespace from the output file. Trailing whitespace is considered bad practice.

What should the trailing digit be? (Please document)

I documented that using an example in: https://github.com/ypid/yaml2rst/blob/feature/fold-marks/docs/fold-markers.rst
But in short. The digit can be used to put the fold on a certain level. If you omit the explicit level, you will have to close the folds with the corresponding closing marker.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would strip foldmarks in every line, not only in rst-comments

It should only affect text lines. Can you double check?

I thought it should only effect comment lines (in rst :-). As of now it effects all text lines, including code lines. E.g. this will remove the fold mark, too:

# Example for using fold.marks::
#
#    Some Section [[[
#    --------------------------------

But maybe this is not a problem. Lets try it.

\s* is of no use?

It will additionally strip any trailing whitespace from the output file. Trailing whitespace is considered bad practice.

IC. For our usecase trailing whitespace is no problem: It's just in memory and rst does not care.. But anyway, we can keep te \s* in.

What should the trailing digit be? (Please document)

I documented that using an example in:

I can not find it there. Please add a short comment (e.g \d are the optional folding levels)

@ypid ypid force-pushed the feature/fold-marks branch 2 times, most recently from 7f74a13 to bac66d2 Compare June 2, 2016 22:12
@ypid
Copy link
Member Author

ypid commented Jun 2, 2016

Sorry that it took me a while to respond. Busy week.

this will break the documentation at http://docs.debops.org/, unless we convert everything at once.

Unfortunately I think you are right. I experienced similar issues when converting ypid.packages to my preferred folding method. I case you intend to make the switch some day, I will be happy to help.

I wonder if there is an alternative to RST that could be more suited for this, but probably not. I'll try to find out if Ansible team has any plans for role documentation.

That would be interesting indeed.

I suggest filtering these marks only if a command-line option tells so. I'd suggest this option to also take the string to be taken as fold-marks, e.g. --strip-foldmarks=[[[,]]]

I agree. But I tried to keep it more generic.

I updated the PR and added a somewhat comprehensive documentation. Refer to https://github.com/ypid/yaml2rst/blob/feature/fold-marks/docs/fold-markers.rst.

As the second variant does not introduce redundancy and is thus preferred, I will use it for my roles. @drybjed What do you think about using this variant for the documentation of DebOps Contrib. So we could use this "Staging area" for testing this. Related to debops/docs#111 as roles in DebOps Contrib will probably get there own isolated docs build.


* ``[[[`` is not expected to appear in Ansible/YAML/Jinja. Current recommendation.

Refer to the `PR about fold markers support <https://github.com/htgoebel/yaml2rst/pull/3>`_ for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this. All relevant docs should go in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I removed it.

@htgoebel
Copy link
Collaborator

htgoebel commented Jun 5, 2016

@drybjed I did not follow the discussion about whether this would break documentation at http://docs.debops.org/, Should I merge it?

@htgoebel htgoebel closed this Jun 5, 2016
@htgoebel htgoebel reopened this Jun 5, 2016
@ypid
Copy link
Member Author

ypid commented Jun 5, 2016

@htgoebel This PR will not break the docs as it only adds a feature flag. When one of the roles in the DebOps core project would be changed to my proposed docs format (dropping the upper section marks) without updating all other docs at the same time, this would break debops/docs and docs.debops.org build but not the individual role docs builds.

@htgoebel
Copy link
Collaborator

htgoebel commented Jun 6, 2016

@ypid I'd like to merge this. May I ask you to rebase the branch to resolve the conflict. To my experience this is easier than merging. Thanks.

@htgoebel htgoebel self-assigned this Jun 6, 2016
ypid added 5 commits June 6, 2016 22:33
It started out when I introduced more and more fold markers to the DebOps project. I used `(((` as marker because it was not highlighted by my editor as being a Jinja2 control sequence. The fold marker as turned out to be not-optimal because it also appears in complex Jinja2 conditions quite often. So @drybjed and me settled on `[[[`. The problem was, however, that we always needed to introduce additional redundancy to the files. Example:

```YAML
---

```

@drybjed and I discussed this and agreed that the best thing would be to patch up `yaml2rst` to filter out the fold marker which would allow it to be directly appended after headings for instance. Example:

```YAML
---

```

With this patch, this input gets converted into:

```reStructuredText
.. vim: foldmarker=[[[,]]]:foldmethod=marker

Default variables
=================

.. contents:: Sections
   :local:

----------------------
  Role configuration
----------------------
```

Can we make this the default behavior or should it be made optional? I limited the regex to only match a very defined set of occurrences.
@ypid ypid force-pushed the feature/fold-marks branch from d4ef649 to e1245be Compare June 6, 2016 20:41
@ypid
Copy link
Member Author

ypid commented Jun 6, 2016

Done. Thanks very much for considering this feature!

@htgoebel htgoebel merged commit 1253bc9 into debops:develop Jun 7, 2016
@htgoebel
Copy link
Collaborator

htgoebel commented Jun 7, 2016

Thanks for implementing it :-)

ypid added a commit to ypid/ansible-owncloud that referenced this pull request Jun 13, 2016
@ypid
Copy link
Member Author

ypid commented Jun 18, 2016

@htgoebel Can you make a release so that I can start using it in Travis tests?

@ypid
Copy link
Member Author

ypid commented Jun 19, 2016

this will break the documentation at http://docs.debops.org/, unless we convert everything at once.

@drybjed I just tested that by symlinking ypid.packages into https://github.com/debops/docs and it just works. So seems like we where talking about a non-existing issue 😉

ypid added a commit to ypid/ansible-resources that referenced this pull request Jul 7, 2016
@ypid
Copy link
Member Author

ypid commented Jul 7, 2016

Status: The proposed format of #3 (comment) has been approved for the DebOps project and https://github.com/debops/test-suite has been switched to yaml2rst develop until a release has been made.

ypid added a commit to ypid/yaml2rst that referenced this pull request Jul 8, 2016
@htgoebel
Copy link
Collaborator

@ypid I'd create a new release ASAP - which make take another two weeks, sorry.

ypid added a commit to ypid/yaml2rst that referenced this pull request Jul 12, 2016
ypid added a commit to ypid/yaml2rst that referenced this pull request Jul 12, 2016
ypid added a commit to ypid/yaml2rst that referenced this pull request Jul 12, 2016
`--strip-regex` was intentionally limited to the RST part of the input
to avoid problems.
`--yaml-strip-regex` allows to mess with the YAML part of the input if
needed.

Related to: debops#3
Related to: debops/docs#156
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.

3 participants