-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add-FilterPrivacy-to-workbookPr #1154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. I've left some comments.
@@ -25,6 +25,8 @@ type SheetPrOptionPtr interface { | |||
} | |||
|
|||
type ( | |||
// FilterPrivacy is an option used for SheetPrOption and WorkbookPrOption | |||
FilterPrivacy bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filterPrivacy
attribute didn't belong to worksheets properties, so I suggest defining it in the workbook.go
, and correct doc comment for it.
@@ -123,6 +123,11 @@ func (o CodeName) setWorkbookPrOption(pr *xlsxWorkbookPr) { | |||
pr.CodeName = string(o) | |||
} | |||
|
|||
// setWorkbookPrOption implements the WorkbookPrOption interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest the get and set implement functions following the order in OOXML Spec, move this function before CodeName
, and update the available options document of the functions SetWorkbookPrOptions
and GetWorkbookPrOptions
// filter privacy of thw workbook. | ||
func (o *FilterPrivacy) getWorkbookPrOption(pr *xlsxWorkbookPr) { | ||
if pr == nil { | ||
*o = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case was not been tested, please add a test case for this.
This fixed code review issue in PR qax-os#1154
This fixed code review issue in PR qax-os#1154
more information on this ticket: #1152