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

add sum_amount_ support to table plugin #179

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shordeaux
Copy link

No description provided.

@m3nu
Copy link
Collaborator

m3nu commented Dec 2, 2018

Since it's your first PR, you should go all the way and add a test case and a sample invoice. I'm having a hard time imagining how this works or when.

Without a test case and test PDF, the feature may be broken by someone else in the future. One more reason to add one. (any confidential content can be redacted or replaced in the PDF)

@m3nu m3nu self-requested a review December 2, 2018 13:48
@rseabrook
Copy link
Contributor

@m3nu what do you think about refactoring to separate field extraction from type conversion and sum field aggregation? The tables plugin is a new way to extract fields but duplicates the type conversion code from the core lib. If the core lib handled those two tasks as the final step before output, the plugins could take advantage of it. That would simplify the plugins and solve this issue in a cleaner way.

@m3nu m3nu assigned m3nu and unassigned m3nu Jun 21, 2019
@rmilecki
Copy link
Collaborator

rmilecki commented Aug 29, 2022

Any special handling of extracted data based on fields names is not a really good idea. It's not flexible enough. It doesn't allow specifying all details on how data should be handled. For the same reason new syntax for fields was introduced.

If we need feature as proposed in this pull request we should probably think about syntax like

tables:
  - start: Hotel Details\s+Check In\s+Check Out\s+Rooms
    end: Booking ID
    body: (?P<hotel_details>[\S ]+),\s+(?P<date_check_in>\d{1,2}\/\d{1,2}\/\d{4})\s+(?P<date_check_out>\d{1,2}\/\d{1,2}\/\d{4})\s+(?P<amount_rooms>\d+)
  - start: Booking ID\s+Payment Mode\s+Amount
    end: DESCRIPTION
    body: (?P<booking_id>\w+)\s+(?P<payment_method>(?:\w+ ?)*)\s+(?P<amount>\d+\.\d+)
    fields:
      amount:
        type: float
        group: sum

@bosd
Copy link
Collaborator

bosd commented Mar 18, 2023

If we need feature as proposed in this pull request we should probably think about syntax like

tables:
  - start: Hotel Details\s+Check In\s+Check Out\s+Rooms
    end: Booking ID
    body: (?P<hotel_details>[\S ]+),\s+(?P<date_check_in>\d{1,2}\/\d{1,2}\/\d{4})\s+(?P<date_check_out>\d{1,2}\/\d{1,2}\/\d{4})\s+(?P<amount_rooms>\d+)
  - start: Booking ID\s+Payment Mode\s+Amount
    end: DESCRIPTION
    body: (?P<booking_id>\w+)\s+(?P<payment_method>(?:\w+ ?)*)\s+(?P<amount>\d+\.\d+)
    fields:
      amount:
        type: float
        group: sum

I agree with @rmilecki on the syntax.
Before I did see this thread I made a quick pr to implement the support of coerce types.
#498

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.

5 participants