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

Renamed XLPivotValues parameters #1946

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Renamed XLPivotValues parameters #1946

merged 3 commits into from
Jan 5, 2023

Conversation

0MG-DEN
Copy link
Contributor

@0MG-DEN 0MG-DEN commented Dec 31, 2022

Fixes #1945

Copy link
Member

@jahav jahav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface IXLPivotValues also needs to have signatures changed.

PivotValueFieldReference needs to have a diferent value in ctor. It wuld fail later in some edge cases, because it uses pt.Values.IndexOf(Value.ToString()) where Value is a SourceName (at least when I looked through call graph).

The AbstractPivotFieldReference.Match method of the is later used for pivot table table styling IIRC, https://github.com/ClosedXML/ClosedXML/wiki/Pivot-Table-Styles#cross-of-values-and-axis-fields

@0MG-DEN
Copy link
Contributor Author

0MG-DEN commented Jan 4, 2023

Thanks for the guidance @jahav
Sorry for not making these changes right away

@0MG-DEN 0MG-DEN requested a review from jahav January 4, 2023 17:38
0MG-DEN and others added 3 commits January 5, 2023 02:19
…ing with the pivot values, because there can be multiple value fields for the same source name (e.g. avg and sum of the same source).
…d (e.g. sum of a Filed or a average of a field) and therefore PivotValueFieldReference should use CustomName to find the correct pivot value field, not SourceName.
@jahav jahav added this to the v0.100 milestone Jan 5, 2023
@jahav jahav merged commit 0df9f69 into ClosedXML:develop Jan 5, 2023
@jahav
Copy link
Member

jahav commented Jan 5, 2023

Thanks for the contribution. Pivot tables will likely get some TLC in 0.102. That IndexOf should probably be removed from public API (position randomly changes on modification of dictionary).

@igitur
Copy link
Member

igitur commented Jan 5, 2023

Pivot tables do need some work, but have also come a long way in a short time, before dev paused.

I'd suggest the next big step is to factor out common pivot table data sources, because multiple pivot tables can reference a single source and changes to a source affect all dependent pivot tables.

Also see related issue at dotnet/Open-XML-SDK#612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Methods' parameters' names in XLPivotValues
3 participants