ctubbsii commented on PR #6460: URL: https://github.com/apache/accumulo/pull/6460#issuecomment-4928740614
> > null is used a lot. Instead of adding a nullable parameter, it'd be better to overload the forFile method with a version that requires the parameter (requireNonNull) and one that omits it. The diff would be a lot smaller that way, as code that passes null wouldn't need to change. > > I agree, but I think I'd rather fix this in a follow-on issue. Breaking the code by adding the parameter allows me to quickly identify broken code and evaluate whether or not a FileStatus for the file already exists. If I add the overloaded method before I merge to main, then I probably have to re-evaluate things I've already looked at. It seems to me that the decision to use null vs non-null parameter is no different than the choice to use an overloaded method. I think now that you've already identified those that can be null, it makes sense to go ahead and add the overloaded method so you can make the patchset smaller and can more strictly enforce the expected parameter. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
