+1 for allow in tests and ban in production code.
- - -- --- ----- -------- ------------- Jacek Lewandowski śr., 30 paź 2024 o 17:37 David Capwell <dcapw...@apple.com> napisał(a): > I am fine with allow in tests ban in src > > On Oct 30, 2024, at 7:16 AM, Štefan Miklošovič <smikloso...@apache.org> > wrote: > > 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 >> >>>>>>> >> >>>>>>> >> >>>>>>> >> > >
