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

Property queries: paren-less or paren-ful? #17128

Closed
vasslitvinov opened this issue Feb 9, 2021 · 9 comments
Closed

Property queries: paren-less or paren-ful? #17128

vasslitvinov opened this issue Feb 9, 2021 · 9 comments

Comments

@vasslitvinov
Copy link
Member

Should methods that return properties of a datatype - such as size, emptiness, etc. - be parenless or have parenthesis?

Consider the range type as an example. It has the following "property" methods:

intIdxType isBounded hasLowBound hasHighBound stride alignment aligned first last low high alignedLow alignedHigh isEmpty size hasFirst isNaturallyAligned isAmbiguous

Should they be parenless or parenful or ...?

Here are key points from our range module review on 2/04:

  • Methods for field-like properties should be paren-less.
  • Methods that can be reasonably expected to perform computation should have parens.
  • The above, however, will leave the user guessing whether a particular property is field-like or computation-like. Even among the range queries above, it is easy to guess incorrectly.

Our resolution was to make this a matter of style -- and state it clearly in the documentation:

  • Methods whose names start with is or has have parens.
  • Methods with the other names are parenless.

Parenthezation in the range module today follows this style. Of particular note are size and isEmpty because they are standard queries among many datatypes in our modules and are parenless (for size) vs. paren-ful (for isEmpty) consistently.

@vasslitvinov
Copy link
Member Author

I personally find the approach of "these are parenless and those are parenful" an unnecessary complication. It would be more elegant if all property queries were either consistently parenless or consistently parenful. Perhaps the former because parens add unnecessary noise to the program text.

@bradcray
Copy link
Member

bradcray commented Feb 9, 2021

Our resolution was to make this a matter of style -- and state it clearly in the documentation:

Methods whose names start with is or has have parens.
Methods with the other names are parenless.
Parenthezation in the range module today follows this style. Of particular note are size and isEmpty because they are standard queries among many datatypes in our modules and are parenless (for size) vs. paren-ful (for isEmpty) consistently.

I'm personally pretty happy with the status quo here. I'd find it pretty annoying if I had to write r.low() rather than r.low and a little odd if I had to write r.isAligned rather than r.isAligned().

So if someone insisted that they all be consistent, then I'd opt for paren-less. But then I'd feel unclear on what the "style guide" was saying; Is it that "all procedures that require no arguments should be written in a paren-less rather than paren-ful style?" To me, that feels like a step too far.

One other note here is that if a routine is paren-ful, additional optional arguments can be added to it over time here, whereas if it's paren-less, we're locked into paren-less for good.

@bradcray
Copy link
Member

This was discussed in today's range review (take 2), and there were no objections to the current parenthesization of these 0-argument queries. Barring further objections, I think we can consider ourself committed to the status quo.

It was noted that we should be consistent when we support these same queries (with similar meanings) on other data structures. For example, we've been talking about supporting .first / .last on domains, arrays, lists, and the like. It was proposed that we should be consistently paren-ful or paren-less in all cases.

@lydia-duncan
Copy link
Member

@bradcray / @vasslitvinov - it sounds like this can be closed, right?

@vasslitvinov
Copy link
Member Author

There are two questions in this issue:

(1) Should we change whether a given range property query is or is not parenthesized?

(2) What is the general guideline for choosing paren-full vs. paren-less style for a method that queries a property of a data type? The goal is to reduce the need for the user to consult the documentation in cases like "should I call myRange.isEmpty with or without parentheses?"

For (1) the consensus is no, keep the status quo instead.

For (2), I have not seen a direct discussion of the resolution described in the OP. Off-issue it generally has received support and matches the style of many existing query methods. Some discussions have come up, however:

  • There has been a move to make the proposed approach recommended, not required.
  • There was a particular 0-arg method for which we discussed whether it counts as a "property query" or not, affecting whether this guideline applies to it. It may have been domain.dims.

I suggest we resolve (2) before closing this issue.

@vasslitvinov
Copy link
Member Author

We could close this issue on the grounds that we are happy with the status quo. However...

There is one case that is an exception: the .first/.last queries, see #17128 (comment)

  • ranges, domains, arrays: parenless
  • lists: parenful

More to look into: head, tail (defined for arrays), low, high (only for ranges and domains?)

@bradcray
Copy link
Member

bradcray commented Jan 4, 2023

head, tail (defined for arrays)

I don't think arrays should continue to support head/tail. I was surprised these wouldn't have come up in our review of the array module, and would guess it's because they're no-doc'd (so arguably not part of the public interface. But it'd be good to deprecate them in any case).

low, high (only for ranges and domains?)

This doesn't particularly concern me. I don't think we'll want to remove them from ranges and domains, and we can always add them to other types later on. I'm not in a rush to do so because they're a bit ambiguous on collections like arrays (i.e., "low index, or lowest (minimum) element?"), and not that interesting on non-array collections (always 0 and size-1, or equivalently .indices.low/.indices.high).

@vasslitvinov
Copy link
Member Author

In today's team meeting we favored strongly "make .first and .last parenless", with a suggestion to do so on all collections. Nobody spoke in favor of the other options.

@lydia-duncan
Copy link
Member

first and last have been handled, closing

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

No branches or pull requests

3 participants