+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
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>>
>
>

Reply via email to