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

Simplify import statements #1614

Closed
wants to merge 1 commit into from
Closed

Conversation

vodik
Copy link
Contributor

@vodik vodik commented May 21, 2018

Removes an unnecessary level of indentation, and changes them from using lists to expressions. See for details #1612

Nice work with the pattern matching compiler, its really easy to make the change.

@Kodiologist
Copy link
Member

Kodiologist commented May 21, 2018

Thanks!

I think we should stick with square brackets. Special forms tend to use parentheses only for inner elements that look like special forms themselves, such as (except …) and (else …). Otherwise, they use square brackets for grouping.

Also, are you sure that (import foo :as bar) is better than (import [foo :as bar])? With (import foo :as bar), related elements among the arguments of import are no longer grouped.

@vodik
Copy link
Contributor Author

vodik commented May 21, 2018

I think we should stick with square brackets.

No strong opinion either way. I'll wait till we have consensus before spending the effort fixing that up.

Also, are you sure that (import foo :as bar) is better than (import [foo :as bar])? With (import foo :as bar), related elements among the arguments of import are no longer grouped.

I was a little skeptical of the change at first, but I do think its an improvement. Your concern is fair, but it just took a little bit of use to learn how to read it quickly, and most of the issues here can be addressed with style guidelines.

Here's where I see there's an issue with writing (import [foo :as bar]) is that we introduce two different internal inconsistencies with the syntax:

  • We write (import [foo :as bar])
  • And we write (import [foo bar :as baz])
  • We don't write (import [foo [bar :as baz]])

We either should put them in on both cases or in neither case (leaning towards neither).

The other important thing to note is that if we force someone to write (import [foo :as bar]), they might not understand what's going on behind the scene and assume that something like (import [foo :as bar baz]) will work to effectively be import foo as bar; from foo import baz, but that won't work. By removing those inter braces, its no longer possible to make that mistake.

As for reading it:

I think its a matter of style. Doing this kind of convention is actually pretty clear imho.

(import (os.path exists
                 isdir :as dir?
                 isfile :as file?)
        sys :as systest
        re)

@Kodiologist
Copy link
Member

Okay, we can lose the extra brackets for :as.

@gilch
Copy link
Member

gilch commented May 22, 2018

Special forms tend to use parentheses only for inner elements that look like special forms themselves

That's not really true of macros though, even in Hy. To the end-user, the distinction is kind of fuzzy. And Clojure has special forms that break this "rule" too (even though import is a macro in Clojure), like letfn.

@Kodiologist
Copy link
Member

Kodiologist commented May 22, 2018

That's not really true of macros though, even in Hy. To the end-user, the distinction is kind of fuzzy.

You mean we have core macros that use parentheses for grouping? We should probably fix that. Although, most macros that expect groups won't distinguish between square brackets and parentheses; you need some code to check for that explicitly. Unless you're using model patterns, but that's not a thing yet.

@gilch
Copy link
Member

gilch commented May 22, 2018

->, ->>, as->, and doto all use () grouping. Like this new import, and unlike try, the possible expression heads are not drawn from a fixed set of pseudo-"special forms".

@Kodiologist
Copy link
Member

Oh, those cases make sense, since the parenthesized groups end up compiled as regular expressions (typically function calls).

@gilch
Copy link
Member

gilch commented May 22, 2018

Argh, OK, think about how this gets indented,

;; status quo, note the indents:
(import [os.path [isdir :as dir?
                  isfile :as file?
                  exists]]
        [sys :as systest]
        re)

;; brackets mean editor indents elements as peers (the head is implicit).
(import [os.path isdir :as dir?
         isfile :as file?
         exists]
        sys :as systest
        re)

;; parentheses mean that the head is special and the rest are arguments.
(import (os.path isdir :as dir?
                 isfile :as file?
                 exists)
        sys :as systest
        re)

The head is special, not a peer. If we drop the nested brackets, we need to change the remaining outer level to parentheses.

@Kodiologist
Copy link
Member

We already disagree how to indent Lisp, so I'm not sure what rhetorical strategy you were aiming for there.

But, I will admit that parens do suggest more than square brackets that the head is special and not a peer. I can only reply that the rule that a parenthesized expression is like a function call (it's a special form, or it's a macro call, or it becomes one of these things, or the like), whereas brackets are used for everything else, seems more salient to me.

However! I think I like your original proposal, #851, best. If we're going to break the world to remove unnecessary punctuation from import and require, we might as go the whole way. The two guys who seemed opposed to that are no longer active on Hy. @vodik, what do you think of it?

@gilch
Copy link
Member

gilch commented May 23, 2018

We already disagree how to indent Lisp

You refuse to learn the standard and won't let your editor do it for you either, you mean. Maybe you enjoy counting brackets. As a meditative exercise. Or maybe you feel the need to write code no-one else can read. For job security.

However! I think I like your original proposal, #851, best.

My proposed syntax from #851, which drops the outer brackets instead of the inner ones,

(import os.path [isdir :as dir?
                 isfile :as file?
                 exists]
        sys :as systest
        re)

The new pattern matching compiler should be able to parse it just as easily.

This version does need the square brackets to indent properly, not parentheses. It also means that as you type, you don't have to even think about the brackets until you need them. With the status quo syntax, I'm frequently frustrated when typing imports at the repl because I have to back up and insert more brackets. I think it's slightly nicer that @vodik's current proposal in that regard, but the current proposal is closer to Clojure's import macro, so there's that.

@Kodiologist
Copy link
Member

You refuse to learn the standard and won't let your editor do it for you either, you mean. Maybe you enjoy counting brackets. As a meditative exercise. Or maybe you feel the need to write code no-one else can read. For job security.

I don't accuse you of indenting code in the way you like for some stupid or nefarious purpose, so please don't do that to me. It's evident (from when you threatened a "hostile fork", from when you unilaterally closed one of my PRs an hour after I'd opened it, and from this) that indentation is a sensitive matter for you. I don't know why that is, but it's not appropriate to take it out on me or anybody else.

@gilch
Copy link
Member

gilch commented May 23, 2018

it's not appropriate to take it out on me

Stop playing the victim, Kodi. Real talk. I can understand your initial mistakes borne of ignorance, but your continued refusal to format your code in a readable way demonstrates that you don't care about wasting our time if it saves you a little. Your hostility toward the rest of the team deserves a response in kind, because you won't care unless someone makes you, by calling you on it. Spinning your personal laziness as a disagreement with me personally was pure spite. Enough of this.

@refi64
Copy link
Contributor

refi64 commented May 23, 2018

Okay, okay, I think we've all had our fun, but just to be clear: indentation is purely, at the end of the day, nothing other than opinion, and I wouldn't consider it something worth seriously debating outside of academic circles. At minimum, it's not worth accusations that will likely be forgetting by next week.

@refi64
Copy link
Contributor

refi64 commented May 23, 2018

We seem to have agreed that #851 is the nicest syntax. Does anyone else (at least, anyone who's actually working on Hy right now) want to object?

@paultag
Copy link
Member

paultag commented May 25, 2018

Stop playing the victim, Kodi. Real talk. I can understand your initial mistakes borne of ignorance, but your continued refusal to format your code in a readable way demonstrates that you don't care about wasting our time if it saves you a little. Your hostility toward the rest of the team deserves a response in kind, because you won't care unless someone makes you, by calling you on it. Spinning your personal laziness as a disagreement with me personally was pure spite. Enough of this.

@gilch I understand there's some amount of disagreement here, and I'll admit that I'm not super active these days, but shit like this is so completely out of line.

I understand you perceive there to be a lot of hostility, and I understand you know that this behavior is unacceptable and used it to try to make a point, but it's 100% not OK. We're all here to work together on something fun. I don't understand why we're at each other's necks.

I don't care about who started what or who disagrees about what, but no one should be writing crap like that about indentation. Readability is important, but getting personal is beyond rational.

Let's make sure that this doesn't happen again, I'm incredibly incredibly sad to read conversations like this in PRs and discussions.

As a community, we've never had to take any steps beyond reminders like this to fix behavior, so I'm fairly confident that we can cut this out without any extra actions needing to be taken.

That being said, I'm going to start keeping an eye on comments on all PRs for a while, I'm really unhappy to read this.

@Kodiologist
Copy link
Member

Well said, guys.

We seem to have agreed that #851 is the nicest syntax. Does anyone else (at least, anyone who's actually working on Hy right now) want to object?

It looks like we're all cool with it. My guess is that Simon would have no objection, but he just hasn't had the chance to return to this PR. Maybe I'll pick it up myself if he doesn't.

@vodik
Copy link
Contributor Author

vodik commented May 27, 2018

👍 on looking nice.

@vodik
Copy link
Contributor Author

vodik commented May 27, 2018

Done.

My only concern with the new syntax, after playing with it, is that maybe this is a little visually ambiguous if the import causes a particular name to be imported or not:

(import sys           ; imports sys
        sys [stdin])  ; imports stdin, not sys - the second expression changes the meaning of the first one

And while I can live with it (so I don't protest too hard but I am officially voting for my original syntax), my previous iteration I think was immune to this problem. And I think its mostly a style issue anyways, as its only really weird when presented like this: (import sys sys [stdin]) - the double sys might throw newbies off in this form - when done as one line, I think (import sys (sys stdin)) is clearer.

@Kodiologist
Copy link
Member

Presumably you would write it in one line as

(import sys  sys [stdin])

or so, using double spaces to separate pairs.

@gilch
Copy link
Member

gilch commented May 27, 2018

How about dropping the space for the one-line version (import sys sys[stdin]) as a matter of style? This is similar to the dict lookups with the dot dsl. (. spam['eggs']), which also look a little more like Python without the space. It even looks OK for multiple from imports if you think of it as a tuple lookup in the module dict like,
(import sys sys[stdin stdout stderr]), especially if we do #1481.

I already approved the first version that dropped the inner brackets (but only because it used () for the outer brackets), but I'd approve my original proposal from #851 too.

@Kodiologist
Copy link
Member

How about dropping the space for the one-line version (import sys sys[stdin]) as a matter of style?

That makes sense, too.

I'd approve my original proposal from #851 too.

That's what Simon has done now, right?

(import tests.resources [kwtest function-with-a-dash])
(os.path exists
isdir :as dir?
isfile :as file?)
Copy link
Member

Choose a reason for hiding this comment

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

I think you missed some parentheses here.

@Kodiologist
Copy link
Member

@vodik, could you rebase onto the new master?

@gilch
Copy link
Member

gilch commented May 27, 2018

using double spaces to separate pairs.

That's my proposal from #1069, which was pretty well-received, but has yet to make it into the style guide. It does make sense to separate semantic groupings this way, like

(import sys  sys [stdin :as sin  stdout :as sout  stderr :as serr])

That is, where you'd use a newline if it were multiple lines.

I think the no-space variant is prettier in this case, like,

(import sys sys[stdin :as sin  stdout :as sout  stderr :as serr])

But we'd keep the double spaces in the [] groups. The no-space rule makes the parings clear at the import level, so we don't need the double space there anymore.

We should probably add the no-space rule for semantic lookups in square brackets to the style guide too.

@vodik
Copy link
Contributor Author

vodik commented May 28, 2018

@gilch like it. Okay, I'll do another pass where we make all the imports have a consistent style then, if we agree on this.

Removes an unnecessary level of indentation, and changes them from
using lists to expressions.

Closes hylang#1612
@Kodiologist
Copy link
Member

Looks like you hit an internal PyPy bug! Nice.

@refi64
Copy link
Contributor

refi64 commented May 28, 2018

Might be a random issue... Out of curiosity, I'll try restarting it just to see.

@refi64 refi64 closed this May 28, 2018
@refi64 refi64 reopened this May 28, 2018
Copy link
Member

@Kodiologist Kodiologist left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm not merging it because it sounds like you want to edit it for spacing first. Either way, you're clear to merge.

@Kodiologist
Copy link
Member

@vodik, how are we doing here?

@Kodiologist
Copy link
Member

I'm closing this because of the lack of follow-up. Please reopen if you want to pick this up again.

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.

5 participants