rouge-ruby / rouge

A pure Ruby code highlighter that is compatible with Pygments
https://rouge.jneen.net/
Other
3.3k stars 732 forks source link

Zenspider/warnings double loads and fixes #1962

Open zenspider opened 1 year ago

zenspider commented 1 year ago

Fixes #1961

This PR fixes >300 warnings that are emitted while loading the code and/or running specs with warnings on.

Please consider reviewing this by commit.

Also consider turning off whitespace changes (for one commit on a rake task file). Tack on ?ws=1.

This is 12 commits, some should be squashed but I separated them because I didn't trust all my changes and this lets it be reviewable by the language owner.

Note: This completely removes use of (but not the definition of) Rouge.load_file and friends. They were not properly preventing double loads and eventually I decided it would be cleaner and easier if everything was just required via ruby to be more idiomatic and quell ~200 redefined method warnings. This does remove all of the "self-mutating" methods and eagerly loads the keyword files. It does not noticeably change load time at all. That might have been relevant when we all had slow spinning hard drives, but not anymore.

zenspider commented 1 year ago

One warning is left for a regexp in julia that I am digging into but might best me:

lib/rouge/lexers/julia.rb:255: warning: character class has duplicated range: /[\p{L}\p{Nl}\p{S}_][\p{Word}\p{S}\p{Po}!]*/

I've figured out that it is it is the last half, and related to the ! being in Po, but when I reduce the regexp the warning goes away but when I put the fix back in the original it is still there... I'm a bit stumped.

jneen commented 1 year ago

Ya know, I've been against it in the past, but I wouldn't mind transitioning to require_relative. The current system comes from a time when ruby's require was painfully slow (and in my defense, require_relative wasn't standard in commonly used ruby versions), but I imagine that's been more or less fixed in the last 10 years.

jneen commented 1 year ago

That being said, I would not trust ruby's -w as some kind of northern star for "good code", especially when it comes to regular expressions. There are a large number of very odd design decisions in there that weren't very thought out IMO.

zenspider commented 1 year ago

That being said, I would not trust ruby's -w as some kind of northern star for "good code", especially when it comes to regular expressions. There are a large number of very odd design decisions in there that weren't very thought out IMO.

Nobody is claiming this... Lack of output from -w doesn't mean "good code", but output from -w is evidence of "bad code" (for varying levels of "bad").

zenspider commented 12 months ago

Where does this stand?

zenspider commented 11 months ago

2 more weeks after months... Do what you want with this PR. I'm divesting myself of it.

jneen commented 11 months ago

Okay!

tancnle commented 10 months ago

Thank you for your hard work on this @zenspider 🙇🏼 ❤️

Sorry I am just catching up to this. I think the require_relative replacement makes sense to me. From the discussion threads above, I don't see there is any strong argument against it 🤔 (please correct me if I have misread). In that case, I think we should roll in the require_relative changes. The remaining can be broken down to smaller MRs so we can branch out further discussions. I can take point on splitting this MR. What do you think @zenspider @jneen?