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

remove spaces and line breaks inside urls #190

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Feb 16, 2023

This single line PR is a proposal about how to handle long urls in docstrings. With this patch

<url>:NetworkX:
     https://networkx.org/documentation/networkx-2.8.8/reference/algorithms/\
      generated/networkx.algorithms.tree.mst.minimum_spanning_edges.html
</url>

the tag is processed as

<a href=https://networkx.org/documentation/networkx-2.8.8/reference/algorithms/generated/networkx.algorithms.tree.mst.minimum_spanning_edges.html>NetworkX</a>

The pros are

  • we do not need to have long lines in documentation (flake8 complains about it)
  • in the code, we can preserve the indentation, and visually looks better.
  • we do not need to change the docstrings that were written like this.

The contra is that I don't know if sphinx supports something like that, so it would make the migration harder.

@@ -122,6 +122,7 @@ def repl_list(match):
def repl_hypertext(match):
tag = match.group("tag")
content = match.group("content")
content = content.replace(" ", "").replace("\n", "")
Copy link
Member

Choose a reason for hiding this comment

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

Please put in a comment as to why this is being done.

@rocky
Copy link
Member

rocky commented Feb 16, 2023

I am on the fence about this, but in an open-source project if this kind of thing makes you happy or removes an annoyance, and you want to do the work to do this fully then sure, let's go ahead.

So note that a change also needs to be made in doc_utils.py of mathics-core. (In fact in looking over the generated LaTeX file is where I was first alerted something was amiss.)

And readthedocs for what is allowed in a URL tag is also needed.

  • we do not need to have long lines in documentation (flake8 complains about it)

flake8 has been told to allow lines up to 300 characters in mathics-core in setup.cfg so that can removed after URL handling allows spaces, if this is done.

  • in the code, we can preserve the indentation, and visually looks better.

yes, this is nice.

  • we do not need to change the docstrings that were written like this.

This is kind of an unusual argument: I didn't understand how things worked, or test what I did, so instead I'll changed the code to match what thought or hoped it would be.

Personally, I suppose I have done this many times myself too, and there is a kind of benefit: this sometimes is how progress is made. However it is not something, personally, I advertise or tout as an advantage.

The contra is that I don't know if sphinx supports something like that, so it would make the migration harder.

This is an interesting point too. Personally, if I didn't know if sphinx supported something, I would look. And if it doesn't I'd ask myself if not then why not? Or if it handles it but in a different way, then I might try to match that rather than go invent a new convention on my own.

Here is situation with Sphinx: docs can live outside of Python code so it is not subject to pylint style. RsT reflows automatically, and you can use long lines.

Redoing everything for Sphinx is indeed going to cause its own challenge. Not unlike the kind we have seen already. However, in my opinion having good docs now is a sad conscious choice I've made.

Cutting over to Sphinx, even forgetting the markup is such a challenge and lower priority than all of those other major problems we have.

@mmatera
Copy link
Contributor Author

mmatera commented Feb 16, 2023

I am on the fence about this, but in an open-source project if this kind of thing makes you happy or removes an annoyance, and you want to do the work to do this fully then sure, let's go ahead.

I open this to discuss if this solution could work. The other way around would be to reinforce the format of the ... tags in the consistency tests.

So note that a change also needs to be made in doc_utils.py of mathics-core. (In fact in looking over the generated LaTeX file is where I was first alerted something was amiss.)

And readthedocs for what is allowed in a URL tag is also needed.

Yes, I am aware of that, and if we decide that this is an acceptable solution, then I am going to put it in the corresponding PRs.

  • we do not need to have long lines in documentation (flake8 complains about it)

flake8 has been told to allow lines up to 300 characters in mathics-core in setup.cfg so that can removed after URL handling allows spaces, if this is done.

OK.

  • in the code, we can preserve the indentation, and visually looks better.

yes, this is nice.

  • we do not need to change the docstrings that were written like this.

This is kind of an unusual argument: I didn't understand how things worked, or test what I did, so instead I'll changed the code to match what thought or hoped it would be.

Again, the other possibility would be to reinforce the docstrings with a test, and fix all the failing cases. At this point, I guess both options would take more or less the same time to be done.

Personally, I suppose I have done this many times myself too, and there is a kind of benefit: this sometimes is how progress is made. However it is not something, personally, I advertise or tout as an advantage.

The contra is that I don't know if sphinx supports something like that, so it would make the migration harder.

This is an interesting point too. Personally, if I didn't know if sphinx supported something, I would look. And if it doesn't I'd ask myself if not then why not? Or if it handles it but in a different way, then I might try to match that rather than go invent a new convention on my own.

The problem with this is that is actually a question for the future: probably at least until the next year we are going to conserve the present documentation format, just because migrating it would take a lot of time, which I have only during hollidays. So, if we think it is useful, sphinx does not support it, we could ask to the developers. But first, we should agree that it is indeed a good idea.

Here is situation with Sphinx: docs can live outside of Python code so it is not subject to pylint style. RsT reflows automatically, and you can use long lines.

Redoing everything for Sphinx is indeed going to cause its own challenge. Not unlike the kind we have seen already. However, in my opinion having good docs now is a sad conscious choice I've made.

Cutting over to Sphinx, even forgetting the markup is such a challenge and lower priority than all of those other major problems we have.

@rocky
Copy link
Member

rocky commented Feb 16, 2023

@mmatera Looks like for this PR, I don't see anything more to do.

If you are up to changing mathics-core and readthedocs to bring them in line with this change. The please go ahead and merge this into master when you are satisfied. And then, at least in the short term (which who knows may be years), is s win.

@mmatera mmatera marked this pull request as ready for review February 16, 2023 18:25
@mmatera mmatera merged commit 733f549 into master Feb 16, 2023
@mmatera mmatera deleted the remove_blanks_in_urls branch February 16, 2023 18:25
mmatera added a commit to Mathics3/mathics-core that referenced this pull request Feb 16, 2023
The same as in Mathics3/mathics-django#190

This single line PR is a proposal about how to handle long urls in
docstrings. With this patch

```
<url>:NetworkX:
     https://networkx.org/documentation/networkx-2.8.8/reference/algorithms/\
      generated/networkx.algorithms.tree.mst.minimum_spanning_edges.html
</url>
```

the <url></url> tag is processed as
```
<a href=https://networkx.org/documentation/networkx-2.8.8/reference/algorithms/generated/networkx.algorithms.tree.mst.minimum_spanning_edges.html>NetworkX</a>
```
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