search

mosra / m.css

A no-nonsense, no-JavaScript CSS framework, site and documentation theme for content-oriented websites
https://mcss.mosra.cz
Other
409 stars 92 forks source link

Upgrade ansilexer color support #151

Closed blairconrad closed 4 years ago

blairconrad commented 4 years ago

Fixes #148.

codecov[bot] commented 4 years ago

Codecov Report

Merging #151 into master will increase coverage by 0.12%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   98.10%   98.22%   +0.12%     
==========================================
  Files          27       27              
  Lines        6477     6547      +70     
  Branches       44       44              
==========================================
+ Hits         6354     6431      +77     
+ Misses        123      116       -7
Impacted Files Coverage Δ
plugins/ansilexer.py 100.00% <100.00%> (+12.72%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 840c11d...533af4c. Read the comment docs.

blairconrad commented 4 years ago

Notes:

  1. I haven't built the docs. I'm a bad person. @sizmailov did, though!
  2. The ansilexer is essentially completely rewritten. Sorry about that.
  3. I made some opinionated choices rather than bombarding you with questions. I will make any alterations you like. Do not hesitate to get me to change anything at all.
  4. One of the perhaps controversial things I did was replace console-colors.ansi with a beefed up version. I did this in a separate commit, so it's easy to drop and restore it.
sizmailov commented 4 years ago

You mentioned you run on windows. Is there a need in testing on linux? I can help with that.

blairconrad commented 4 years ago

Oh, that's kind, @sizmailov. I started trying to set up an environment on WSL and got sidetracked. (I'm secretly wondering whether a layer of build automation couldn't be applied to the repo, but that's another discussion). I think the only tests I'd've affected are for the code plugin, and I was able to confirm that they still pass. If you wanted to run something, you could build the docs and make sure the Math and Code section renders… I should really try to set myself up to build.

sizmailov commented 4 years ago

Apparently travis-ci integration is broken and github doesn't show builds. Here is direct link. I'll look into it locally.

blairconrad commented 4 years ago

Ah! Thanks for the link. It looks like I broke some doxygen tests that involve ansi codes. Probably I just need to update the reference file. I can look at it later today. https://travis-ci.org/github/mosra/m.css/jobs/695973732#L752

sizmailov commented 4 years ago

Indeed, I've run tests locally and replaced files, which made almost all test pass (tests with math result to arbitrarlly formatted svg, so they never run successfully on my machine). I've submitted a PR to you PR :)

blairconrad commented 4 years ago

Oh, thanks, @sizmailov. In parallel, I'd fixed some of the tests and updated files. Your changes are much better. I am inclined to take them. Am struggling with how to merge in while preserving your credit. Stay tuned for updates.

sizmailov commented 4 years ago

Don't worry about credits, they are not worth wasted time.

blairconrad commented 4 years ago

@sizmailov said

Don't worry about credits, they are not worth wasted time.

I feel somewhat bad about this, but would also say exactly the same thing in your situation, so I've incorporated your changes, with thanks! Unsurprisingly, we have success! https://travis-ci.org/github/mosra/m.css/builds/696083412

mosra commented 4 years ago

Wow, man. This is beyond great, I'm impressed.

I'll remove the ZWNJ, fix the conflict (caused by me updating license headers) and merge. Thanks a lot.

mosra commented 4 years ago

Merged as f3a166100128107527a0335c6912c4bc9754831e (I removed the ZWNJ hack before in d1837841fc4705f311a106c2efe553f811c46a9d, to have this patch clean). Thanks again!

blairconrad commented 4 years ago

Thanks for the kind words, @mosra, and the opportunity to contribute (for my own benefit, as I'm the one who was complaining). And thanks again @sizmailov. I'd've never've known I broke tests without you.

I should probably try to get a Linux VM or WSL or something running…

mosra commented 4 years ago

With this, m.css has ANSI color code parsing better than most CIs (neither Travis, CircleCI nor AppVeyor were able to properly decode everything I wanted them to), so thank you :)

Regarding testing -- I'm fine if you abuse the CI for that (I'm an extreme on the other side, maintaining a project that runs on Windows and booting Windows only once or twice a year, mostly just relying on the CIs for that). Apologies that it's broken and doesn't report the build status back, I don't know what's up.

Btw., one thing I noticed in the "ground truth" output:

<span class="g g-AnsiDefault" style="background-color: #5f00d7">

(And then repeated several times for all the colors.) Isn't the g g-AnsiDefault redundant in this case? I didn't look close enough to see if this is really an explicitly specified default in the input (in which case I'd say it's okay), but if not then I think it isn't needed. Or is this a nested override of something defined above and thus it needs to be there?

blairconrad commented 4 years ago

You... may be right. The g g-AnsiDefault might be superfluous here. I can take a look tonight or tomorrow.

mosra commented 4 years ago

Thanks! It's not a critical issue, just that it generates output that's heavier than it could be.