Satwik-Bhardwaj commented on code in PR #967:
URL: https://github.com/apache/poi/pull/967#discussion_r2607873904
##########
poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java:
##########
@@ -1061,11 +1061,30 @@ public HSSFSheet getSheetAt(int index) {
* If there are multiple matches, the first sheet from the list
* of sheets is returned.
*
+ * <p>
+ * Note that Excel limits sheet names to 31 characters. If you try to
look up
+ * a sheet by a name longer than that, a warning will be logged, since
+ * such a sheet cannot exist in Excel and the lookup will likely
return {@code null}.
+ * </p>
+ *
* @param name of the sheet
* @return HSSFSheet with the name provided or {@code null} if it does not
exist
*/
@Override
public HSSFSheet getSheet(String name) {
+
+ if (name == null || name.isEmpty()) {
+ return null;
+ }
+
+ if(name.length() > MAX_SENSITIVE_SHEET_NAME_LEN) {
+ LOGGER.atWarn().log(
Review Comment:
> why log something, why not throw an exception?
My original choice of logging a warning was specifically to maintain
backward compatibility and prevent breaking existing user code upon upgrading
the library. Since the method currently doesn't throw an error for this input,
a sudden exception would halt production code.
However, I agree that throwing an IllegalArgumentException would be the
stricter, cleaner approach for long-term API correctness.
Which path does the team prefer? Are we prioritizing backward compatibility
(Warning) or strictly enforcing input validity (Exception/Breaking Change)? I
am happy to implement either direction.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
