Closed paulo-ferraz-oliveira closed 2 years ago
Best viewed with "Hide whitespace changes" 😉
The action is not running yet, because it hasn't been merged to master
.
This is probably also best viewed commit by commit, for less context switching.
If this gets accepted, I won't mind "fixing" the open pull requests that are getting updated. This is a "for the future" thing, if you will.
@garazdawi, I didn't think this would move forward actually (had no high hopes), but glad something can. So, the goal is to compare the before and after HTML and make sure it's the same?
I'm not sure why the HTML entities change with just white space changes, but I didn't dig deep into build.pl
.
I am, however, making sure the HTML differences, in the now-generated files, aren't relevant, one by one.
I'm going to commit a new bunch of white space changes (basically), some for consistency within the document itself, some for fixing issues with the previous changes.
EEP 7, EEP 9 and EEP 43 contain a lot of errors.
A lot of non-valid HTML? Unreadable? I mean, I did find some "errors" but not "a lot", so I might not be looking in the correct place/at what you're looking at.
Also, I did pass my eyes over all .md and then the generated .html and nothing seemed very wrong, but I will re-generate and try to figure out if there's more stuff to change.
Here's a brief summary of the recent changes:
<blockquote>
changes<br>
fixes<p>
fixesA lot of non-valid HTML? Unreadable? I mean, I did find some "errors" but not "a lot", so I might not be looking in the correct place/at what you're looking at.
No, not invalid HTML. Places where the removal/addition of whitespace changed the content. What I mostly saw was code examples that became incorrectly indented after the fixes, which it seems like you have caught in your review.
My definition of "a lot" was "more than I can feel like going through on a Friday evening", which to be honest was not really all that many :)
Would you mind rebasing on top of latest master? I'll take a look through your changes once that is done and see if I can find any other places that look odd.
I'll keep this on my TO-DO list (might be done in the next 5 min.s or the next 5 days, not sure when I'll have time - or how long it'll take 😄).
First bit is done. That was easier then I though. I'm now going over all merges to master
to make sure I didn't skip anything wrongly.
Notes:
High-level API
to [#high-level-api](High-level API)
(inner link)Some other (hopefully) small changes were done. I'm going to look at the .mds on GitHub now.
I'm moving away from draft. The new relevant commit is 200c4e5. I tried to make sure all changes look good in HTML and Markdown, but it's not easy to appease both at the same time 😢. For HTML, I did the minimum required to make it look readable. This introduced some whitespace in the .md
s' visual output, but it might be Ok.
- 0007.md: changed
High-level API
to[#high-level-api](High-level API)
(inner link)
Markdown.pl does not support inner links, so this will turn into a dead anchor in the final HTML... which is not what we want I think.
It looks a lot better now. I added two commits that fixes two issues that I found.
I saw, thanks.
I think this may be ready to merge. I'll run another pass through everything tomorrow and see if I catch anything. Also, just so that you are aware, my plan is for us to move from using Markdown.pl to https://commonmark.org/ as the specification for how EEPs are rendered.
That's good, I hope. As long as it's more modern. If you tell me the plan, though, I may try to tackle it. Is it Perl? Or do you have something different in mind?
Markdown.pl does not support inner links, so this will turn into a dead anchor in the final HTML... which is not what we want I think.
Yeah, sorry, I didn't catch ALL of it, it's a lot of stuff. In any case, yeah, what looks good in .md
might not in .html` (is there a real interest in maintaining the HTML version? I'm convinced that some of what's already in the repo. was never reviewed as HTML 😄).
- 0007.md: changed
High-level API
to[#high-level-api](High-level API)
(inner link)Markdown.pl does not support inner links, so this will turn into a dead anchor in the final HTML... which is not what we want I think.
I'll revert this change.
That's good, I hope. As long as it's more modern. If you tell me the plan, though, I may try to tackle it. Is it Perl? Or do you have something different in mind?
I haven't really gotten as far as having a plan yet :) just a goal!
The commonmark reference implementation is in C if I recall correctly, but there are implementations of it in python and js.
Yeah, sorry, I didn't catch ALL of it, it's a lot of stuff. In any case, yeah, what looks good in .md might not in .html` (is there a real interest in maintaining the HTML version? I'm convinced that some of what's already in the repo. was never reviewed as HTML smile)
When you say "what looks good in .md", you probably really mean "what looks good in github flavored markdown".
The ./build.pl
version is the way that the EEP spec defines that the markdown should be rendered. So the way that github renders the markdown is (right now) not as important as how the ./build.pl
HTML looks like. If we move to commonmark the difference will be smaller, but there will still be a difference https://github.github.com/gfm/#what-is-github-flavored-markdown-
When you say "what looks good in .md", you probably really mean "what looks good in github flavored markdown".
I do. I supposed I thought (probably wrongly) that the target was this, since this is where EEPs are being stored. But it is HTML 👍
I've reverted all the whitespace changes that might be relevant. I'll point out those that might not, though, since some of them might've been there by mistake to start with (I'm not sure of the original author's motivation).
I've reverted all the whitespace changes that might be relevant. I'll point out those that might not, though, since some of them might've been there by mistake to start with (I'm not sure of the original author's motivation).
When viewing your revert using the github GUI it does a very good job at highlighting the places where trailing whitespace can be removed, while not showing anything for where they are significant.
I'll take a last look at this tomorrow and then should be ready to merge.
I'm happy with the changes. Thanks for going through this so thoroughly.
All done! Thanks!
@garazdawi, here's a rationale on the chosen configuration options: