Use rouge for highlighting instead of Pygments #141

kylef · 2015-05-08T17:12:26Z

This removes a dependency on a Python project, and greatly improves syntax highlighting along with fixing #138.

Before:

After:

This does add a required dependency which may not be desired, if that is the case I'm happy to amend this pull request to make rouge optional.

houndci-bot · 2015-05-08T17:16:00Z

lib/xcpretty/syntax.rb

-    register_filetype 'dylan',  ['.dyl', '.dylan']
-    register_filetype 'ruby',   ['.ruby', '.rb']
+      if !lexer && File.extname(options[:filename]) == '.m'
+        # Objective-C is hard to guess when you use BDD style frameworks like Kiwi and Specta


Line is too long. [93/80]

kattrali · 2015-05-08T22:44:01Z

The output is definitely nicer

houndci-bot · 2015-05-19T16:54:06Z

lib/xcpretty/syntax.rb

-      `echo '#{code}' | pygmentize -f 256 -l #{language} #{options if options}`
-    end
+      if !lexer && File.extname(filename) == '.m'
+        # Objective-C is hard to guess when you use BDD style frameworks like Kiwi and Specta


Line is too long. [93/80]

houndci-bot · 2015-06-24T22:00:27Z

lib/xcpretty/syntax.rb

+begin
+  require 'rouge'
+rescue LoadError
+  Rouge = nil


Use SCREAMING_SNAKE_CASE for constants.

kylef · 2015-06-24T22:02:40Z

I've updated this pull request to make rouge optional (it's still in the gem spec, so when installed via ruby gems it will be available).

supermarin · 2015-06-25T22:48:48Z

lib/xcpretty/syntax.rb

+        :filename => File.basename(filename),
+        :source => snippet.contents,
+      }
+      lexer = Rouge::Lexer.guesses(options).first


Wouldn't it be better to just use :filename => filename and not let it guess from the contents?
I believe that'd be faster as well

It's not as accurate since certain pieces of code might have false positives against other languages. Especially when using BDD style frameworks.

I don't think performance is much of a problem since this code path is only called when there is a failing test (and there shouldn't be many of these ;)). This is also how it worked before so it was acceptable beforehand.

Hmm.. I was playing with filenames and sources a bit, and seems like it sometimes recognizes objc (.m extensions), and always works with swift.

@kylef I appreciate code removal (and am always for it), but not sure if we should maybe keep the manual filename mapping.
What do you think?

I think both should be here, and here's a few examples of why:

This code using the Quick/Nimble Swift framework isn't able to be detected as Swift, purely because it would also be valid syntax in other languages such as Ruby and Python. The file extension is required to improve the guessing.

Filename isn't enough to match Objective-C, since .m is a file extension shared with other file types (matlab for one).

supermarin · 2015-06-25T23:35:05Z

@kylef sorry, maybe my explanation was bad.

Wouldn't it be better to just use :filename => filename and not let it guess from the contents?

This means using the whole filename (e.g. file.swift) instead of File.basename(filename) that you're using ATM.

I am aware that guess* methods are not 100% accurate, that's why I proposed to keep the manual mapping and just tell Rouge which language is it.

kylef · 2015-06-25T23:39:21Z

Okay that makes sense, I'll try address this soon.

supermarin · 2015-06-25T23:46:03Z

Thanks for your work! appreciate that!

supermarin · 2015-10-06T17:48:37Z

xcpretty.gemspec

@@ -26,6 +26,8 @@ Gem::Specification.new do |spec|
  spec.test_files    = spec.files.grep(%r{^(test|spec|features)/})
  spec.require_paths = ["lib"]

+  spec.add_dependency 'rouge', '~> 1.8'
+


Sorry if we already discussed - should we try to implicitly rely on Rouge like we did on Pygments?
That way we're still zero dependencies and will colorize if someone has installed Rouge

This is already conditional, the conditionally depends on rouge in code.

return snippet.contents unless Rouge

I put a hard-dependency in the gemspec since I would assume if you are using RubyGems to install xcpretty (a package manager) you would want it to manage your packages and dependencies.

From #141 (comment)

@kylef if there's a hard dependency, then there's no need for the conditional :)
The good thing is that it has no dependencies, and that's something we valued a lot in xcpretty itself so far.

On the other hand - I think we should be moving towards homebrew because xcpretty should be treated like a command line tool rather than a gem.
I understand we can bundle that up together with rouge tho, just thinking out loud.

@kattrali what are your thoughts on this particular one?

@kylef sorry just saw your comment ^^

So far rubygems are the only method one can install xcpretty with, afaik :)

Sorry for the confusion guys, multitasking :)
Let me try once again - if I fail let's just 🚢 🇮🇹 .

Currently we use RubyGems, which is bad.
We should be using Homebrew and treat xcpretty like a CLI tool rather than a library.

No dependencies is great because install time is fast, and rubygems are super slow with dependencies->dependencies->dependencies.

Not having that line in .gemspec gives you the possibility of just installing xcpretty and manually installing rouge.
If we put that line into .gemspec, you'll have Rouge installed on each Travis job.

If we put that line into .gemspec, you'll have Rouge installed on each Travis job.

You wouldn't, both Circle CI and Travis CI pre-install xcpretty in their OS X images.

I've removed it from the gem spec in 336e394 since you feel strongly about it not being there. But this means that RubyGems will not lock the version of Rouge and any breaking changes could cause xcpretty to break for users either now or in the distant future.

If the user has Rouge < 1.8 it may not work, or >= 2.0.

We should be using Homebrew and treat xcpretty like a CLI tool rather than a library.

Isn't that an even bigger dependency? One that requires you to be an administrator of your computer. Many users may not be able install xcpretty if they are in corporate or education environments where they do not have administrator privileges.

This conversation is off-topic and doesn't belong on this PR...

Due to 336e394, I don't think this is a good idea and I think we should close this PR.

Ok, I think we're on the same page this time.

Currently we use RubyGems, which is bad.
We should be using Homebrew and treat xcpretty like a CLI tool rather than a library.

Considering there are plenty of other tools already in the average cocoa developer's toolbox which are installed via rubygems (and its on every OS X system already), this still seems negotiable. There are no barriers to installation; rubygems handles libraries with a binary components just fine, just as homebrew, pip, or any other package manager does. Even then, if you bundle xcpretty without rouge via Homebrew, it still works, just doesn't have rouge.

No dependencies is great because install time is fast, and rubygems are super slow with dependencies->dependencies->dependencies.

No argument here, however in this case we are talking about one dependency which greatly enhances the usability and has no dependencies itself. There is the chance that otherwise the rouge gem installed on the system does not meet this requirement and breaks xcpretty as a result when we do implicit dependencies.

Due to 336e394, I don't think this is a good idea and I think we should close this PR.

I'm sorry if I caused a conversation, that wasn't the intention.
Thank you again for your work - it's a great thing and we should merge this PR.

houndci-bot · 2015-10-06T20:29:04Z

lib/xcpretty/syntax.rb

+      else
+        options = {
+          :filename => File.basename(filename),
+          :source => contents,


Use the new Ruby 1.9 hash syntax.
Avoid comma after the last item of a hash.

Use rouge for highlighting instead of Pygments

0xced · 2015-10-08T07:41:56Z

While the default theme (ThankfulEyes) works fine on a dark background, it’s terrible on a light background.

Is there a way to detect the terminal background color so that we could use an appropriate theme?

supermarin · 2015-10-08T14:40:10Z

Pretty hard; maybe there's a color scheme that works better.
I think the main problem is that it uses hardcoded color codes instead of basic terminal codes.

houndci-bot reviewed May 8, 2015
View reviewed changes

houndci-bot reviewed May 19, 2015
View reviewed changes

supermarin mentioned this pull request Jun 24, 2015

Disable gems while running #145

Closed

houndci-bot reviewed Jun 24, 2015
View reviewed changes

lib/xcpretty/syntax.rb

begin

require 'rouge'

rescue LoadError

Rouge = nil

Copy link

Collaborator

houndci-bot Jun 24, 2015

Choose a reason for hiding this comment

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

Use SCREAMING_SNAKE_CASE for constants.

supermarin reviewed Jun 25, 2015
View reviewed changes

supermarin reviewed Oct 6, 2015
View reviewed changes

kattrali modified the milestone: 1.0.0 Oct 6, 2015

kylef added 2 commits October 6, 2015 13:28

Use rouge for highlighting instead of Pygments

5545fae

Allow usage when rouge isn't installed

c2adf9e

houndci-bot reviewed Oct 6, 2015
View reviewed changes

kylef added 2 commits October 6, 2015 13:33

Hard-code known supported file types

bb617cc

[Rubocop] Disable Style/ConstantName for fallback import

a5e06a2

kattrali added a commit that referenced this pull request Oct 6, 2015

Merge pull request #141 from kylef/kylef/rouge

9b047b7

Use rouge for highlighting instead of Pygments

kattrali merged commit 9b047b7 into xcpretty:master Oct 6, 2015

kylef deleted the kylef/rouge branch October 6, 2015 20:43

kattrali mentioned this pull request Oct 6, 2015

Swift string interpolation causes xcpretty to show provide syntax errors #138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rouge for highlighting instead of Pygments #141

Use rouge for highlighting instead of Pygments #141

kylef commented May 8, 2015

houndci-bot May 8, 2015

kattrali commented May 8, 2015

houndci-bot May 19, 2015

houndci-bot Jun 24, 2015

kylef commented Jun 24, 2015

supermarin Jun 25, 2015

kylef Jun 25, 2015

supermarin Jun 25, 2015

supermarin Jun 25, 2015

kylef Jun 25, 2015

supermarin commented Jun 25, 2015

kylef commented Jun 25, 2015

supermarin commented Jun 25, 2015

supermarin Oct 6, 2015

kylef Oct 6, 2015

kylef Oct 6, 2015

supermarin Oct 6, 2015

supermarin Oct 6, 2015

supermarin Oct 6, 2015

kylef Oct 6, 2015

kylef Oct 6, 2015

kattrali Oct 6, 2015

supermarin Oct 6, 2015

houndci-bot Oct 6, 2015

0xced commented Oct 8, 2015

supermarin commented Oct 8, 2015

Use rouge for highlighting instead of Pygments #141

Use rouge for highlighting instead of Pygments #141

Conversation

kylef commented May 8, 2015

Choose a reason for hiding this comment

kattrali commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylef commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supermarin commented Jun 25, 2015

kylef commented Jun 25, 2015

supermarin commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xced commented Oct 8, 2015

supermarin commented Oct 8, 2015