imvtsl commented on PR #84: URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2734966459
Late here, but I wanted to put forth my point: > the parameter withReplacement in the other cases already contains the "with" part in the name. I believe the original name SampleSeed better aligns with Go naming conventions and the existing API pattern. In the function SampleWithReplacement, the "With" is included because it directly corresponds to the boolean parameter withReplacement, which defines the sampling behavior. In contrast, the SampleSeed function takes a seed parameter (not a withSeed), so adding "With" would introduce an unnecessary preposition that doesn't accurately describe the argument. Go naming conventions generally favor concise, clear names that directly reflect their parameters and purpose. By following this convention, SampleSeed is consistent with both Go idioms and the naming pattern established by SampleWithReplacement. Here is Google's style guide about naming conventions that you can reference: [https://google.github.io/styleguide/go/best-practices.html#naming](https://google.github.io/styleguide/go/best-practices.html#naming) > @imvtsl Golang does not have the concept of default parameters or method overloading. The idiomatic way of dealing with this is to provide different methods. Using an int pointer to indicate optionality is not common. > > For example, Show() does not have optional parameters in spark-connect-go. I understand that Go doesn't support default parameters or method overloading. However, I believe the current implementation of `Show()` might not be ideal. In PySpark, the show() method supports optional parameters: `n` (int, optional): Number of rows to show. `truncate` (bool or int, optional): If True, truncates strings longer than 20 chars by default. If set to a positive integer, truncates long strings to that length and aligns cells right. `vertical` (bool, optional): If True, prints output rows vertically (one line per column value). However, the current implementation of `Show()` in `spark-connect-go` forces clients to provide all parameters, even if they just want to use default behavior. This isn't correct. As you mentioned, the idiomatic approach in Go is to provide different methods instead of relying on optional parameters. Just like I did for the Sample function, we could have separate methods for `Show()` to support different use cases without forcing clients to pass all parameters. -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
