rtpsw commented on PR #13252: URL: https://github.com/apache/arrow/pull/13252#issuecomment-1150927804
> Most of these changes are minor but we do need to clean up the constructors. > > I realized as I was doing the review that some of these method comments (e.g. `\brief`) were following existing patterns in this file. However, I think the suggestions move us more in line with the rest of the code base (e.g. `\brief` should be a one line description. The things that follow are a note) and we can clean up the others later. > > I find the nested ternary (i.e. `(a ? b : c) & d` a little too compact to follow. Now that `Status && Status` is unavailable I think we should probably avoid `&` when short-circuit is desired when using `Status`. > > It might be worth noting somewhere (assuming my understanding is correct here) that even if `allow_overwrite` is true it a registry will never modify its parent. > > Lastly, I'm not sure why we didn't check for existing function names in `AddAlias`. I think we should probably correct that here rather than persist the status quo. But it's possible I'm not understanding some nuance. I agree with almost everything. I think your comments about logic are not minor :) I ended up fixing and cleaning up the code quite a bit following them. Thanks for catching. -- 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]
