Change non-existent hl_linenos to hl_lines to allow passthrough in safe mode #3787

Merged
merged 1 commit into from Aug 4, 2015

Conversation

Projects
None yet
4 participants
@TWiStErRob
Contributor

TWiStErRob commented Jun 14, 2015

Fixes #3776 by changing to the correct name for whitelisting.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

Did you manually test this works? Also it seems you broke tests, can you fix those please? 👍

Contributor

envygeeks commented Jun 15, 2015

Did you manually test this works? Also it seems you broke tests, can you fix those please? 👍

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jun 15, 2015

Contributor

I manually modified my installed version of jekyll in the Ruby gems directory and tested it on my site if it goes through. I don't want to set up a whole environment for jekyll dev, it was enough to do it as a jekyll user. It's probably the same fix in the test as well, just changing the constant. I'll do it tomorrow and see what CI says.

Contributor

TWiStErRob commented Jun 15, 2015

I manually modified my installed version of jekyll in the Ruby gems directory and tested it on my site if it goes through. I don't want to set up a whole environment for jekyll dev, it was enough to do it as a jekyll user. It's probably the same fix in the test as well, just changing the constant. I'll do it tomorrow and see what CI says.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

Without the tests working merging this is an automatic 👎, we cannot accept anything you change if you break tests and don't fix those tests that defeats the purpose of testing. I understand it's hard to modify software as a non-programmer but that does not change that we need software to have quality.

Contributor

envygeeks commented Jun 15, 2015

Without the tests working merging this is an automatic 👎, we cannot accept anything you change if you break tests and don't fix those tests that defeats the purpose of testing. I understand it's hard to modify software as a non-programmer but that does not change that we need software to have quality.

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jun 15, 2015

Contributor

Yep, I agree with testing. I'm a dev too, just in the strongly typed world. I'm just lazy to set up yet another development environment for changing a typo.

Contributor

TWiStErRob commented Jun 15, 2015

Yep, I agree with testing. I'm a dev too, just in the strongly typed world. I'm just lazy to set up yet another development environment for changing a typo.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

For Ruby it's easy to isolate all that mess. When you clone our repo do bundle install --path vendor/bundle so you can isolate all the dependencies and then script/test will do the rest for you. When you are done remove vendor/bundle and you'll have removed everything we use in dev.

Contributor

envygeeks commented Jun 15, 2015

For Ruby it's easy to isolate all that mess. When you clone our repo do bundle install --path vendor/bundle so you can isolate all the dependencies and then script/test will do the rest for you. When you are done remove vendor/bundle and you'll have removed everything we use in dev.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 15, 2015

Member

👍 to what @envygeeks suggested; it's very easy to develop Jekyll.

Even if you don't want to set it up locally, you can use Travis's output to fix the tests.

Member

parkr commented Jun 15, 2015

👍 to what @envygeeks suggested; it's very easy to develop Jekyll.

Even if you don't want to set it up locally, you can use Travis's output to fix the tests.

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jun 15, 2015

Contributor

bundle install --path vendor/bundle

Thanks, I like that better than installing gems into my ruby I'll never use. Just like npm's node_modules.

and then script/test

Exactly my point. Executing that would require me to install cygwin or a virtual machine. Luckily I was able to decypher that I need ruby -S bundle exec rake test, which obviously thrown an SIGSEGV (https://bugs.ruby-lang.org/issues/10978), why would it work?!..., then I tried the other option ruby -S bundle exec ruby -Itest which didn't use any processor for minutes, so I closed it. To get rake work I would have to compile ruby myself, because my 2.2.2 is 500 revs behind... I hope you see what I mean now by not wanting to set up the env.

you can use Travis's output to fix the tests.

Yep, that's the only way now; that was my original idea when I said "see what CI says.". @envygeeks' description got my hopes up again that it's really "that simple".

Contributor

TWiStErRob commented Jun 15, 2015

bundle install --path vendor/bundle

Thanks, I like that better than installing gems into my ruby I'll never use. Just like npm's node_modules.

and then script/test

Exactly my point. Executing that would require me to install cygwin or a virtual machine. Luckily I was able to decypher that I need ruby -S bundle exec rake test, which obviously thrown an SIGSEGV (https://bugs.ruby-lang.org/issues/10978), why would it work?!..., then I tried the other option ruby -S bundle exec ruby -Itest which didn't use any processor for minutes, so I closed it. To get rake work I would have to compile ruby myself, because my 2.2.2 is 500 revs behind... I hope you see what I mean now by not wanting to set up the env.

you can use Travis's output to fix the tests.

Yep, that's the only way now; that was my original idea when I said "see what CI says.". @envygeeks' description got my hopes up again that it's really "that simple".

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jun 15, 2015

Contributor

Any idea for this failure? it looks unrelated.

Contributor

TWiStErRob commented Jun 15, 2015

Any idea for this failure? it looks unrelated.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

@TWiStErRob You could use boot2docker on Windows to solve all of that, I should get around to building a dev image for people but I've been putting it off since I have an active project.

That said I retriggered the test and I'll update you once I see if it's a Travis fluke or consistent.

Contributor

envygeeks commented Jun 15, 2015

@TWiStErRob You could use boot2docker on Windows to solve all of that, I should get around to building a dev image for people but I've been putting it off since I have an active project.

That said I retriggered the test and I'll update you once I see if it's a Travis fluke or consistent.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

Seems to me it was a fluke, all is well. 👍

Contributor

envygeeks commented Jun 15, 2015

Seems to me it was a fluke, all is well. 👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 15, 2015

Member

What was the error?

Member

parkr commented Jun 15, 2015

What was the error?

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jun 15, 2015

Contributor

@parkr JRuby test VS _config.yml location, like this: https://travis-ci.org/jekyll/jekyll/jobs/65915927

Contributor

TWiStErRob commented Jun 15, 2015

@parkr JRuby test VS _config.yml location, like this: https://travis-ci.org/jekyll/jekyll/jobs/65915927

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 15, 2015

Contributor

It's a fluke that happens every once in a while on Travis CI only. I've not been able to reproduce it at all on my local build (but I also don't use RVM... but it happens and I've been trying to get it reproduce ever since.)

Contributor

envygeeks commented Jun 15, 2015

It's a fluke that happens every once in a while on Travis CI only. I've not been able to reproduce it at all on my local build (but I also don't use RVM... but it happens and I've been trying to get it reproduce ever since.)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 18, 2015

Contributor

Is there anything hold this back or can we go ahead and merge it? I'm 👍

Contributor

envygeeks commented Jun 18, 2015

Is there anything hold this back or can we go ahead and merge it? I'm 👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 18, 2015

Member

Do we have a test for this that actually runs it through Pygments? I'm fine on changing it but I'd like a test so we don't break it again.

Member

parkr commented Jun 18, 2015

Do we have a test for this that actually runs it through Pygments? I'm fine on changing it but I'd like a test so we don't break it again.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 18, 2015

Contributor

That's what stalled me, I tried to do this myself on a branch before @TWiStErRob did it and when I added in a test to actually test the output, it always remained unchanged. I do agree though, we probably need a regression tester so it doesn't happen again.

Contributor

envygeeks commented Jun 18, 2015

That's what stalled me, I tried to do this myself on a branch before @TWiStErRob did it and when I added in a test to actually test the output, it always remained unchanged. I do agree though, we probably need a regression tester so it doesn't happen again.

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Aug 4, 2015

Contributor

What's up with this? Is it waiting on me?

Contributor

TWiStErRob commented Aug 4, 2015

What's up with this? Is it waiting on me?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 4, 2015

Contributor

@TWiStErRob if I recall right this is waiting for you to add a test to ensure it's ran properly.

Contributor

envygeeks commented Aug 4, 2015

@TWiStErRob if I recall right this is waiting for you to add a test to ensure it's ran properly.

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Aug 4, 2015

Contributor

I updated the commit, it has the test change too, ci tests were failing at first, because I didn't modify the tests. As for those regression tests, I'm not sure how to even start.

Contributor

TWiStErRob commented Aug 4, 2015

I updated the commit, it has the test change too, ci tests were failing at first, because I didn't modify the tests. As for those regression tests, I'm not sure how to even start.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 4, 2015

Member

ci tests were failing at first, because I didn't modify the tests. As for those regression tests, I'm not sure how to even start.

Ensure you have rebased on the latest master, too, and are using script/cibuild to run the tests.

Member

parkr commented Aug 4, 2015

ci tests were failing at first, because I didn't modify the tests. As for those regression tests, I'm not sure how to even start.

Ensure you have rebased on the latest master, too, and are using script/cibuild to run the tests.

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Aug 4, 2015

Contributor

Rebased, but I still have Windows...

Contributor

TWiStErRob commented Aug 4, 2015

Rebased, but I still have Windows...

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 4, 2015

Member

Yep windows will cause problems. As long as Travis passes, I think we're good.

Member

parkr commented Aug 4, 2015

Yep windows will cause problems. As long as Travis passes, I think we're good.

parkr added a commit that referenced this pull request Aug 4, 2015

@parkr parkr merged commit b4ac044 into jekyll:master Aug 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Aug 4, 2015

@TWiStErRob TWiStErRob deleted the TWiStErRob:hl_lines branch Aug 4, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.