JoshCheek / atom-seeing-is-believing

Seeing is Believing integration for the Atom text editor.
Do What The F*ck You Want To Public License
62 stars 4 forks source link

File Grammar constraint is too strict #32

Closed penntaylor closed 7 years ago

penntaylor commented 7 years ago

I use a semantic highlighting plugin that provides variant grammars. In this case, I use a grammar named "semanticolor - Ruby". SiB worked fine with this variant grammar until most recent update to SiB 2.2.0. Now I get a big red warning message with the text "Seeing is Believing expects a Ruby file" anytime I try to SiB-annotate a document.

Is there a way to add other grammars to the filter that is throwing this error?

Screenshot of full error message: siberror

penntaylor commented 7 years ago

Caught a moment to look into the SiB plugin code. I changed isntRuby to the following to get around the issue. No pull request because I'm not sure what problems led to the need to filter strictly in the first place, so there's a good chance I'm overlooking something important!

isntRuby: (editor) ->
    not editor.getGrammar().name.toLowerCase().match(/ruby/)
JoshCheek commented 7 years ago

You can fix it immediately by just replacing the body of that function with true. (it should be located in ~/.atom/packages/seeing-is-believing/lib/seeing-is-believing.coffee)


Hmm :/ I have to think about what the right solution here is. Probably the right solution is to remove the code that does this. It's trying to be helpful, but seems like there are valid use cases that violate its expectation.

Initially I had SiB just trigger when it was on a Ruby scope, but there were a lot of situations where people expected it to work, but they weren't in a Ruby scope. So then I had it work everywhere, but that just felt wrong (ie it means the keybinding is globally applicable, so you're likely going to hit keybinding conflicts). So it was trying to be helpful by explaining what the issue was and how to fix it. Now, historically, I've found that code which does this is deeply problematic, and you've illustrated that this truth holds here, too. It's just hard to figure out how to make it help new users discover how to use it, and also be flexible enough for experienced users to configure it to work correctly.

The options for a real fix seem to be:

If you reply back and I don't see it, ping me on twitter as I mostly neglect my gh issues and email.

JoshCheek commented 7 years ago

Okay, after a cigarette, I realized that I can just change the matcher to match a regex like /ruby/i But, even that's probably not good enough, because shouldn't it work in a Ruby codeblock inside Markdown? Of course, it doesn't now anyway, b/c it will pull the entire file, so that implies it should only pull the bit that Atom has scoped to /ruby/i, or if there is none, then it should assume that it can't figure it out but the user probably did it on purpose and pull the whole file.

Does that seem reasonable? It feels like it should be, but my brain does not seem to be working so well today :/

JoshCheek commented 7 years ago

For the moment, I just moved the constraint to a regexp, which should work for you (I tested it out with the semanticolor plugin). It's published as a patchlevel update.

I'm not sure that this is the right solution, but it's enough to close this issue. If another user hits it, I'll link the two issues together and try to find the use case that best fits the common set of requirements.

penntaylor commented 7 years ago

What about making the regexp itself a setting that defaults to /ruby/i but can be overridden in the settings panel for anyone who has more complicated needs? On the one end of the spectrum it has the same effect as toggling the behavior on and off (/ruby/i vs /./), and at the other end it allows for narrower filtering if that's needed. Particularly for the latter case, it also survives package upgrades.

On the other hand it might be a cryptic setting that just generates confusion.