Blacken project

[a-recknagel]

Formatting issues come up every now and then, it's nice to get rid of them by using black. I created https://github.com/pyviz/panel/tree/ar/blacken where I ran black with no config and only touched up a few params in viewable.py to have something we can look at to identify issues.

[jbednar]

Thanks! I don't mind most of the changes, but I dislike what it does with Parameter definitions:

The blackened version is great at revealing all the little details of the Parameter definition, but that's precisely the wrong focus, because what people should be able to do with a Parameter list is to easily scan the names of the parameters and read their docs; for nearly all Parameters that's enough to help you realize what knobs there are to turn and whether they are relevant to you. A nice compact block of parameter definitions that all fit on one screen is really helpful for understanding how to use a particular class. It's only after locating the relevant parameter that one would ever wonder about things like allow_None, etc., and even then only rarely. So I want formatting that jumbles those other rarely used things into as small a space as possible, available for people who are looking for them, but not distracting from the parameter name, type, and docstring, which in the original formatting are always in repeatable, easily scanned locations. Anyway, I'm not the BDFL of this particular project, so that's just my opinion...

[a-recknagel]

@jbednar I see. My personal opinion would be that the gain of adopting black would still be greater than losing the readability/conciseness here. But if it's only a few places, we can just do this:

    # fmt: off
    css_files = param.List(default=_CSS_FILES, doc="""
        External CSS files to load as part of the template.""")

    js_files = param.Dict(default={}, doc="""
        External JS files to load as part of the template. Dictionary
        should map from exported name to the URL of the JS file.""")

    raw_css = param.List(default=[], doc="""
        List of raw CSS strings to add to the template.""")

    _embed = param.Boolean(default=False, allow_None=True, doc="""
        Whether plot data will be embedded.""")

    _embed_json = param.Boolean(default=False, doc="""
        Whether to save embedded state to json files.""")

    _embed_json_prefix = param.String(default='', doc="""
        Prefix for randomly generated json directories.""")

    _embed_save_path = param.String(default='./', doc="""
        Where to save json files for embedded state.""")

    _embed_load_path = param.String(default=None, doc="""
        Where to load json files for embedded state.""")

    _inline = param.Boolean(default=True, allow_None=True, doc="""
        Whether to inline JS and CSS resources.
        If disabled, resources are loaded from CDN if one is available.""")

    _truthy = ['True', 'true', '1', True, 1]
    # fmt: on

Would that be better?

[a-recknagel]

@jbednar I pushed another commit where I added the above illustrated fmt: off/on blocks around param definitions that also featured doc sections. Did I miss anything important or add anything superfluous?

[jbednar]

Nearly every class should have parameters with docs, so it would not be just a few places. The # fmt directives seem quite distracting to me. Is there no way to tell Black to avoid reformatting Parameter declarations, as a general rule rather than on a per-class basis? Anyway, @philippjfr would be the one to make the call about whether to do this.

[a-recknagel]

Is there no way to tell Black to avoid reformatting Parameter declarations

no, the configuration options are very minimal. The only one that can be used for this particular purpose is said fmt declaration.

Another option could be to add the doc as an actual docstring to the variable, that would avoid any formatting concerns regarding call signatures because it's not part of the call anymore, and would arguably be even more visible.

    css_files = param.List(default=_CSS_FILES)
    """External CSS files to load as part of the template."""

    js_files = param.Dict(default={})
    """External JS files to load as part of the template. 
       Dictionary should map from exported name to the URL of the JS file."""

    raw_css = param.List(default=[])
    """List of raw CSS strings to add to the template."""

    _embed = param.Boolean(default=False, allow_None=True)
    """Whether plot data will be embedded."""

    _embed_json = param.Boolean(default=False)
    """Whether to save embedded state to json files."""

    _embed_json_prefix = param.String(default='',)
    """refix for randomly generated json directories."""

    _embed_save_path = param.String(default='./')
    """Where to save json files for embedded state."""

    _embed_load_path = param.String(default=None)
    """Where to load json files for embedded state."""

    _inline = param.Boolean(default=True, allow_None=True)
    """Whether to inline JS and CSS resources.
       If disabled, resources are loaded from CDN if one is available."""

    _truthy = ['True', 'true', '1', True, 1]

A downside would be that not everyone knows that variables can have docstrings too and might be confused, and also the whole technical aspect of whether it is possible at all and how hard it would be that I haven't researched yet.

[jbednar]

Unless I'm confused or something changed in later Python versions (the design of Param is from 2003), variables can't have docstrings in the same way as classes and modules do, because they don't define a namespace in which the docstring is available introspectively. Sure, a code or AST parser like Sphinx can use variable docstrings, but Param docstrings are used by the code itself for online help, tooltips (hover info), etc., and need to be available at runtime. If you know a way around that, I'd love to hear it!

[a-recknagel]

No, you're right. Only class and module definitions get to have proper docstrings =/

That's it for in-code solutions from me. Final final option that comes to mind would be to introduce a __repr__ to the Param base class that just returns self.docs, and refer people who want to inspect these classes to the autodocs, which would look like this

No idea whether that would serve the same purpose though, and it would require setting up autodocs as well - but it would allows us to use black freely without fmt blocks. @philippjfr your call on if it should be done at all. And if yes, with or without format-free blocks for some/all the Param definitions.

[ceball]

Here are my opinions (at least my opinions this morning ;) )...

0. This whole topic is an unrelenting, unproductive sideshow...but it never seems to stop coming up.
1. All holoviz projects should do the same thing, whatever that is.
2. Although there are some pretty annoying drawbacks (such as the spacing out of parameter definitions), I'm overall in favor of picking a tool to handle formatting and just living with it.
3. If we were to start using a tool, we should apply it across the whole of a project in one go, even to old code. (Although that pains me - will mess up blame etc.)
4. If we don't adopt a tool, we should add contributor guidelines about formatting. Saying things like please do not change existing whitespace on lines unrelated to your PR (otherwise there'll be spurious diffs which apparently github can't filter out in PR views), please follow the existing conventions in the file/or as documented at link xyz when adding new code, etc etc. (E.g. a refresh of http://ioam.github.io/topographica/Developer_Manual/coding.html)

I think the reason I'm ultimately ok with giving up 'superior'/custom formatting for the consistency of a tool is mainly that I hope to see most parameter definitions being written outside of holoviz projects by non-holoviz developers (one day :) ). People doing that will be using standard IDEs and tools - they are probably not going to follow (undocumented?) suggestions for how to format parameter definitions. I.e. if parameters don't work well with standard IDEs and tools, that's actually sort of our problem. (E.g. it could potentially mildly harm the adoption of parameters if everyone sees other people's code containing large, spaced out blocks of parameters with weird param options taking up a whole line of their own.)

[MarcSkovMadsen]

One vote in favor of Black. To me the right side of the example is actually much more readable.

[jbednar]

I work on a laptop and tend to be very jealous of vertical space, so I don't like any formatting that stretches things out so that I have to page through several screens to find anything. I greatly value being able to have a compact section that lists the parameters available, so that I can study it and decide what this component allows and how to work with it. That said, I'm not the main programmer on any of these projects any more, so I'm happy to leave it up to a democratic vote.

[mattpap]

I work on a laptop and tend to be very jealous of vertical space (...)

I sometimes work with a display in portrait mode and even then I prefer the vertical space saving formatting. In general I would like to see the most vertical content possible, to better understand the broad context of the code being analyzed and/or modified. The later formatting may be better for a casual user or as documentation, but source code is not documentation. I think we should view black for what it is, a serialization format for Python's source code. It's easier on tooling, but that can be alleviated by choosing the right ones (e.g. using diff-highlight to improve git diff's output quality).

(...) so I'm happy to leave it up to a democratic vote.

I think this should be up to people who spend considerable amount (majority?) of their time with the code base. I will only recommend not to adopt black, but I'm far from being comfortable taking part in a vote. I would be very annoyed if someone decided for me how to format bokeh's source code. Given how much time we, project developers, spent with those code bases each and every day, we must be comfortable with what we see on screen.

[jbednar]

Right; the context is why I am anxious to preserve vertical space. It's so easy to get lost when scanning through a file where only a small window is visible at any one time, and so hard to compare one thing to the other if it has to be done serially.

In any case, we would need to use a single style across all HoloViz projects (which does not include Bokeh), and so the people who would participate in such a vote would be the core developers in those projects as a whole. I think that's Philipp, Jean-Luc, me, and probably that's it, when all projects are taken into account. Maybe Chris? Maybe Jon? Basically if Philipp and Jean-Luc were to agree on a style I don't think I or anyone else could or should outvote them.

[philippjfr]

In any case, we would need to use a single style across all HoloViz projects

I'm not sure this is necessary, different projects have different groups of contributors and different legacies. I'm probably going to start typing Panel for instance but don't see that happening for HoloViews, so there is already going to be divergences. While I certainly would argue for and work towards keeping a consistent style across projects, as the de facto BDFL of Panel I do feel like this is in the end my decision to make, even if our other projects do not end up adopting it, either because we don't feel it's appropriate everywhere or because we end up having disagreements about it.

As for the specific case of black formatting, I'm not in love with all of it but overall I've been moving towards a style that is closer to black than the style that our earlier projects have generally used (without actually following an explicit style guide). Personally I have no issues with vertical space because I do most of my work on multiple 4k monitors these days but I agree that it's probably the biggest issue with black. So the only reason why I am even remotely tempted to use black is because I would appreciate not having to worry about formatting when reviewing user PRs. In any case I think we do need to decide on a consistent style for Panel and hopefully other HoloViz projects, but in the absence of a tool to apply that style I feel it's kind of hopeless which I guess brings me back around to using black.

I'd also like to hear from @xavArtley about his preference since he's the other person who has significantly contributed to the project.

[ceball]

I too usually am looking at a screen without much vertical space, and do not particularly like the black formatting above. However, my editor can deal with that kind of stuff (and already has to deal with it for increasingly more and more python code I am reading from elsewhere...), so my second priority is really consistency (my first priority is the main contributors' happiness 😂).

[MarcSkovMadsen]

My first priority is also the main contributor's happiness and that it is awesome to work together on contributing to this project.

One thing you can configure in Black is the number of allowed characters in a line. The people I talk to prefer line width 100. Would that or an even higher number solve the laptop problem?

[xavArtley]

A tool for automate formatting will be great. But after testing black I'm not a big fan. I would perfer a tool wich may be maximize the number of character dispayed on a screen (with a limit of 100/120 character by line). As I worked a lot on my personal laptop with only a 13' display I found black forcing a lot of scrolling.

Le sam. 1 févr. 2020 à 05:07, Marc Skov Madsen notifications@github.com a écrit :

My first priority is also the main contributor's happiness and that it is awesome to work together and contributing to this project.

One thing you can configure in Black is the number of allowed characters in a line. The people I talk to prefer line width 100. Would that or an even higher number solve the laptop problem?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/holoviz/panel/issues/693?email_source=notifications&email_token=AENMGS7URL23DAQDNFWEBMTRATYQFA5CNFSM4JAC2ITKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQS7SY#issuecomment-580988875, or unsubscribe https://github.com/notifications/unsubscribe-auth/AENMGS452QZGXA4YR5NSNKLRATYQFANCNFSM4JAC2ITA .

[MarcSkovMadsen]

Thanks @xavArtley . Have you tried black with a limit of 100 or 120 characters? And would that work for you?

[xavArtley]

My prinicipal concern with black is the way it unwrap all parameters in all function definitions. I don't know if it can be customized.

[mattpap]

I don't know if it can be customized.

No. The point of black is to not give any room for customization, with the exception for line length.

[analog-cbarber]

The problem with black is that it is just too fascist. It only formats things it's way and provides almost no customization (and is pretty militant about that).

If you are going to introduce a code formatting tool, use one that is configurable and actually implements the style you want without forcing you to compromise.

[nritsche]

The problem with black is that it is just too fascist. It only formats things it's way and provides almost no customization (and is pretty militant about that).

If you are going to introduce a code formatting tool, use one that is configurable and actually implements the style you want without forcing you to compromise.

I think that's the big strength of black. No discussion, black decides how the code is formatted.

[analog-cbarber]

It is ONLY a strength to the extent that black does the "right" thing. So if you don't like some of its choices, you shouldn't use it.

One thing you learn if you program for many years in many different languages is that strict formatting rules really don't matter that much and that what makes code readable often depends on the situation.