Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use highr by default and depend on it #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use highr by default and depend on it #77

wants to merge 1 commit into from

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Aug 17, 2016

as discussed in jupyter/nbconvert#356

@jankatins
Copy link
Contributor

jankatins commented Aug 17, 2016

IMO it's not good to add a dependency on a relative uncommon package for such a minor feature. To get repr adopted in other projects it's IMO better to keep the dependency list as small and "already installed" as possible.

IMO it would be better to simply use it if it is available and the FAQ or description item suggest "installing higher will make displayed functions highlighted".

@flying-sheep
Copy link
Member Author

flying-sheep commented Aug 17, 2016

you mean with the highlight = TRUE thing but without highr, we get unhighlighted stuff? idk…

what’s the harm? the hadleyverse has millions of dependencies and you were happy to install them

@jankatins
Copy link
Contributor

jankatins commented Aug 17, 2016

I'm actually happy to never get any highlights, so I wouldn't mind that the option is simply removed and the function just tests for the highr namespace and if it is not present does not do any highlighting ("progressive enhancement" -> currently the option is misleading as there is actually no way to stop highlighting: it's either done by highr or by the latex code). Or print a warning if the namespace isn't there when highlight=TRUE is explicitly set (which doesn't work because now TRUE is the default).

The ham is getting repr adopted somewhere else so that other packages see the benefits of including repr_xxx.<obj>. To get that it's IMO better not to depend on any other package. Right now it's basically only what is included here and getting more package to include their own repr functions proved not an easy task. My feeling is that irkernel is either so unknown that it's not attractive to do something for it or even seen as the "python guys invading the R world" :-(

Another way could be to split off "non essential" implementations from the "declaration" into their own "repr_implementations" package, but that would remove any incentive for other package to include repr functions for their own objects.

@takluyver
Copy link
Member

I don't have strong feelings either way on this, but I'd mention that it should be unnecessary once we implement jupyter/nbconvert#363, which will allow kernels to provide a hint for the frontend to highlight output. That will take a little while, though, so we may still want this in the meantime.

@flying-sheep
Copy link
Member Author

flying-sheep commented Aug 18, 2016

The ham is getting repr adopted somewhere else so that other packages see the benefits of including repr_xxx.<obj>. To get that it's IMO better not to depend on any other package

the fact that you seem to have rather strong feelings towards including a completely inconsequential dependency hints at you being right.

but still: this is R. my “barebones” R installation has 173 packages in it, all interdepending (i removed the recommended and base packages for fairness):

deps

That will take a little while, though, so we may still want this in the meantime.

that’s the point yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants