Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two meanings return in NewSheet(name string) int #714

Closed
Charnnarong opened this issue Oct 18, 2020 · 6 comments
Closed

Two meanings return in NewSheet(name string) int #714

Charnnarong opened this issue Oct 18, 2020 · 6 comments
Labels
bug Something isn't working

Comments

@Charnnarong
Copy link

I would like to thank to all contributors for creating such a great library. It is tremendous help in my work. I have a minor issue here.

https://github.com/360EntSecGroup-Skylar/excelize/blob/520aa679f34bafbc00626151075b0b123eceb516/sheet.go#L36

The comment says Returns the number of sheets in the workbook (file) after appending the new sheet. However, the return number has two meanings.

  1. sheet count
  2. sheet index

For instance

// f *excelize.File
totolSheet := f.NewSheet("newSheetName")
log.Println("create new sheet: ", totolSheet, f.SheetCount)

output.
totalSheet is always one less than f.SheeetCount if "newSheetName" hasn't existed before (succeed in appending a new sheet).
Both are equal in case of "newSheetName" is already exist.

I faced the problem by reading the comment and wrote my code as
tempIndex := f.NewSheet(tempName) - 1
now, I have to change to
tempIndex := f.NewSheet(tempName)
which isn't quite having the same meaning with the function comment.

@Charnnarong Charnnarong changed the title Two meaning return Two meanings return Oct 18, 2020
@Charnnarong Charnnarong changed the title Two meanings return Two meanings return in NewSheet(name string) int Oct 18, 2020
@xuri
Copy link
Member

xuri commented Oct 19, 2020

Thanks for your feedback, as you say the index returned by NewSheet when the worksheet already exists is wrong, it shouldn't be sheet count, I'll fix it.

@xuri xuri added the bug Something isn't working label Oct 19, 2020
@Charnnarong
Copy link
Author

Great, thank you for the quick action! Returning the index is more coherence. Just a kind reminder of updating the comment to reflect the change would be very appreciated.

@xuri xuri closed this as completed in 4834a05 Oct 19, 2020
@xuri
Copy link
Member

xuri commented Oct 19, 2020

I have fixed it, please try to use the master branch code.

@Charnnarong
Copy link
Author

Thank you for the fix. How about the method's comment. The original one is

// NewSheet provides function to create a new sheet by given worksheet name.
// When creating a new spreadsheet file, the default worksheet will be
// created. Returns the number of sheets in the workbook (file) after
// appending the new sheet.

The last sentence Returns the number of sheets in the workbook (file) after appending the new sheet.
Shouldn't it be Returns the index of the sheets in the workbook (file) after it appended ? Or anything similar as you see fit as we are not returning the number of sheets in the workbook anymore.

@xuri
Copy link
Member

xuri commented Oct 22, 2020

I have updated the docs of the NewSheet.

@Charnnarong
Copy link
Author

Charnnarong commented Oct 22, 2020

Thank you for the fix. The comment is now crystal clear. And also with your additional of the default worksheet named ``Sheet1`` will be created. is left no user with no double what it is going to happen when calling the function. Thank you again.

EugeneAndrosovPaser pushed a commit to ceearrashee/excelize that referenced this issue Nov 14, 2020
…ex returned by NewSheet in some case, fix panic on formatted value with no built-in number format ID
jenbonzhang pushed a commit to jenbonzhang/excelize that referenced this issue Oct 22, 2023
…ex returned by NewSheet in some case, fix panic on formatted value with no built-in number format ID
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants