https://issues.apache.org/jira/browse/CASSANDRA-20038
On Wed, Oct 30, 2024 at 2:40 PM Brandon Williams <dri...@gmail.com> wrote:
> Allow in tests, forbid elsewhere is my vote and simple to implement.
>
> Kind Regards,
> Brandon
>
> On Wed, Oct 30, 2024 at 6:46 AM Štefan Miklošovič
> <smikloso...@apache.org> wrote:
> >
> > against vars:
> >
> > Benjamin
> > Stefan
> > Benedict
> > Yifan
> >
> > somewhere in the middle:
> >
> > Dinesh
> >
> > Caleb +1 in tests and +0 in prod code.
> > David - against flat out banning, limiting it just for some places
> >
> > Josh prod / test on/off switch.
> >
> > for:
> >
> > Jon
> >
> > Correct me if I am wrong and I have "miscategorized" somebody.
> >
> > From what I see, I think the outcome is that we might use this in tests
> and let's just forbid it in prod code. My reasoning behind this is that we
> might all learn how to live with this in tests and we may re-evaluate in
> the future if its usage proved to be beneficial and we are comfortable to
> have it in prod code as well (or we go to ban its usage in tests too). In
> other words, if the majority just can't live without this in prod code we
> can take a look at this again in the future.
> >
> > Not banning it in tests does not mean that from now on we need to use
> vars there. It just means that tests which will introduce the usage of vars
> will not be rejected.
> >
> > Does this sound reasonable to everybody?
> >
> > On Wed, Oct 30, 2024 at 12:31 PM Benjamin Lerer <ble...@apache.org>
> wrote:
> >>
> >> I recently faced some var usage in the CQL layer part where it was
> making the code pretty hard to understand. I am in favor of prohibiting it.
> >>
> >> Le mar. 29 oct. 2024 à 20:32, Caleb Rackliffe <calebrackli...@gmail.com>
> a écrit :
> >>>
> >>> Josh's example of "good" usage seems defensible, because the declared
> type is already obfuscated to Collection anyway, and my eyeballs are going
> to skip to the right anyway to see the instantiated type. I'm +0 on
> prohibiting it in non-test code, and -1 on prohibiting it in tests.
> >>>
> >>> On Tue, Oct 29, 2024 at 2:23 PM Jon Haddad <j...@rustyrazorblade.com>
> wrote:
> >>>>
> >>>> When I first saw var as a Java keyword, my initial reaction was one
> of skepticism for the reasons already mentioned. However, in practice,
> I've been using var for years across many code bases (Java and Kotlin) and
> have never had an issue with readability. I find it to be significantly
> more pleasant to work with.
> >>>>
> >>>> Jon
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On Tue, Oct 29, 2024 at 12:13 PM Štefan Miklošovič <
> smikloso...@apache.org> wrote:
> >>>>>
> >>>>> I get that there might be some situations when its usage does not
> pose any imminent problem but the question is how we are going to enforce
> that _at scale_? What is allowed and what not ... Sure we can have a code
> style for that, but its existence itself does not guarantee that everybody
> will adhere to that _all the time_. People are people and make mistakes,
> stuff is overlooked etc.
> >>>>>
> >>>>> Upon the review we will argue whether "this particular usage of var
> is meaningful or not" and "if the codestyle counts with this or not" and as
> many reviews we will have there will be so many outcomes. Dozens of people
> contribute to the code and more than that reads it, everybody has some
> opinion about that ... Sure, just use var when you prototype etc but I
> don't think it is a lot to ask to merge it without vars. Come on ...
> >>>>>
> >>>>> On Tue, Oct 29, 2024 at 8:08 PM Yifan Cai <yc25c...@gmail.com>
> wrote:
> >>>>>>
> >>>>>> I am in favor of disallowing the `var` keyword.
> >>>>>>
> >>>>>> It does not provide a good readability, especially in the
> environments w/o type inference, e.g. text editor or github site.
> >>>>>>
> >>>>>> It could introduce performance degradation without being noticed.
> Consider the following code for example,
> >>>>>>
> >>>>>> Set<String> allNames()
> >>>>>> {
> >>>>>> return null;
> >>>>>> }
> >>>>>>
> >>>>>> boolean contains(String name)
> >>>>>> {
> >>>>>> var names = allNames();
> >>>>>> return names.contains(name);
> >>>>>> }
> >>>>>>
> >>>>>> Then, allNames is refactored to return List later. The contains
> method then runs slower.
> >>>>>>
> >>>>>> List<String> allNames()
> >>>>>> {
> >>>>>> return null;
> >>>>>> }
> >>>>>>
> >>>>>>
> >>>>>> - Yifan
> >>>>>>
> >>>>>> On Tue, Oct 29, 2024 at 11:53 AM Josh McKenzie <
> jmcken...@apache.org> wrote:
> >>>>>>>
> >>>>>>> (sorry for the double-post)
> >>>>>>>
> >>>>>>> Jeremy Hanna kicked this link to a style guideline re: inference
> my way. Interesting read for those that are curious:
> https://openjdk.org/projects/amber/guides/lvti-style-guide
> >>>>>>>
> >>>>>>> On Tue, Oct 29, 2024, at 2:47 PM, Josh McKenzie wrote:
> >>>>>>>
> >>>>>>> To illustrate my position from above:
> >>>>>>>
> >>>>>>> Good usage:
> >>>>>>>
> >>>>>>> Collection<String> names = new ArrayList<>();
> >>>>>>>
> >>>>>>> becomes
> >>>>>>>
> >>>>>>> var names = new ArrayList<String>();
> >>>>>>>
> >>>>>>>
> >>>>>>> Bad usage:
> >>>>>>>
> >>>>>>> Collection<String> names = myObject.getNames();
> >>>>>>>
> >>>>>>> becomes
> >>>>>>>
> >>>>>>> var names = myObject.getNames();
> >>>>>>>
> >>>>>>>
> >>>>>>> Effectively, anything that's not clearly redundant in assignment
> shouldn't use inference IMO. Thinking more deeply on this as well, I think
> part of what I haven't loved is the effective splitting of type information
> when constructing generics:
> >>>>>>>
> >>>>>>> Map<InetAddressAndPort, RequestFailureReason>
> failureReasonsbyEndpoint = new ConcurrentHashMap<>();
> >>>>>>>
> >>>>>>> vs.
> >>>>>>>
> >>>>>>> var failureReasonsByEndpoint = new
> ConcurrentHashMap<InetAddressAndPort, RequestFailureReason>();
> >>>>>>>
> >>>>>>>
> >>>>>>> I strongly agree that we should optimize for readability, and I
> think using type inference to the extreme of every case where it's
> allowable would be the opposite of that. That said, I do believe there's
> cases where judicious use of type inference make a codebase more readable
> rather than less.
> >>>>>>>
> >>>>>>> All that said, accommodating nuance is hard when it comes to style
> guidelines. A clean simple policy of "don't use type inference outside of
> testing code" is probably more likely to hold up over time for us than
> having more nuanced guidelines.
> >>>>>>>
> >>>>>>> On Tue, Oct 29, 2024, at 2:19 PM, Štefan Miklošovič wrote:
> >>>>>>>
> >>>>>>> Yes, for now it is pretty much just in SAI. I wanted to know if
> this is a thing from now on or where we are at with that ...
> >>>>>>>
> >>>>>>> I am afraid that if we don't make this "right" then we will end up
> with a codebase with inconsistent usage of that and it will be even worse
> to navigate in it in the long term.
> >>>>>>>
> >>>>>>> I would either ban its usage or allow it only in strictly
> enumerated situations. However, that is just hard to check upon reviews
> with 100% accuracy and I don't think there is some "checker" to check
> allowed usages for us. That being said and to be on the safe side of things
> I would just ban it completely.
> >>>>>>>
> >>>>>>> Sometimes I am just reading the code from GitHub and it might be
> also tricky to review PRs. Not absolutely every PR is reviewed in IDE, some
> reviews are given without automatically checking it in IDE too and it would
> just make life harder for reviewers if they had to figure out what the
> types are etc ...
> >>>>>>>
> >>>>>>> On Tue, Oct 29, 2024 at 7:10 PM Brandon Williams <dri...@gmail.com>
> wrote:
> >>>>>>>
> >>>>>>> On Tue, Oct 29, 2024 at 12:15 PM Štefan Miklošovič
> >>>>>>> <smikloso...@apache.org> wrote:
> >>>>>>> > I think this is a new concept here which was introduced recently
> with support of Java 11 / Java 17 after we dropped 8.
> >>>>>>>
> >>>>>>> To put a finer point on that, 4.1 has 3 hits, none of which are
> valid,
> >>>>>>> while 5.0 has 172. If 'sai' is added to the 5.0 grep, 85% of them
> are
> >>>>>>> retained.
> >>>>>>>
> >>>>>>> Kind Regards,
> >>>>>>> Brandon
> >>>>>>>
> >>>>>>>
> >>>>>>>
>
