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

Features/WithGroupedHeadingRow #3575

Merged
merged 31 commits into from
Mar 10, 2022
Merged

Features/WithGroupedHeadingRow #3575

merged 31 commits into from
Mar 10, 2022

Conversation

VFAndrew
Copy link
Contributor

@VFAndrew VFAndrew commented Mar 9, 2022

1️⃣ Why should it be added? What are the benefits of this change?
Adds concern WithGroupedHeadingRow to allow import sheets to group data values in an array when multiple columns share the same header key

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No

3️⃣ Does it include tests, if possible?
Yes

4️⃣ Any drawbacks? Possible breaking changes?
No

5️⃣ Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Take note of the contributing guidelines.
  • Checked the pull requests to ensure that another person hasn't already submitted a fix.
  • Added tests to ensure against regression.
  • Updated the changelog

6️⃣ Thanks for contributing! 🙌

@patrickbrouwers
Copy link
Member

Thanks for the PR, looks good. Can you also update the changelog?

@VFAndrew
Copy link
Contributor Author

VFAndrew commented Mar 9, 2022

Thanks for the PR, looks good. Can you also update the changelog?

Whoops, closed my previous PR to keep your package commit history as clean as possible and forgot to include the changelog again.

@VFAndrew
Copy link
Contributor Author

VFAndrew commented Mar 10, 2022

@patrickbrouwers Should be good for further review now.

I also made WithGroupedHeaders concern an extension of WithHeadingRow to enforce the heading row being used. I believe that makes sense but let me know if I should revert that change.

Maybe I should update the concern to WithGroupedHeadingRow to imply this extension?

@patrickbrouwers
Copy link
Member

I also made WithGroupedHeaders concern an extension of WithHeadingRow to enforce the heading row being used. I believe that makes sense but let me know if I should revert that change.

That's a good idea, nice!

Maybe I should update the concern to WithGroupedHeadingRow to imply this extension?

Yes, that sounds better, great suggestion!

@VFAndrew VFAndrew changed the title Features/WithGroupedHeaders Features/WithGroupedHeadingRow Mar 10, 2022
@patrickbrouwers patrickbrouwers merged commit 98cbe86 into SpartnerNL:3.1 Mar 10, 2022
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.

3 participants