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

Added new command 'summary' #1127

Merged
merged 12 commits into from
Jun 10, 2022
Merged

Added new command 'summary' #1127

merged 12 commits into from
Jun 10, 2022

Conversation

shashigharti
Copy link
Contributor

  • Added new command named 'summary'.
  • Added to_summary function to ReportTask.
  • Added tests for both 'summary' command and 'to_summary' function.
  • Reorganized the commonly used function to helpers.

* Added command named summary
* Added to_summary function to ReportTask
* Added tests for both 'summary' command and 'to_summary' function
@shashigharti shashigharti requested a review from roll June 7, 2022 07:17
@shashigharti
Copy link
Contributor Author

The output looks like this:
summary data/countries.csv
output

For to_summary

for task in report.tasks:
    summary = task.to_summary()
    print("# Describe \n")
    print(summary['describe'])
    print("\n# Extract \n")
    print(summary['extract'])
    print("\n# Validate \n")
    print(task.valid)
    print("\n# Summary \n")
    print(summary['validate']['summary'])
    print("\n# Errors \n")
    print(summary['validate']['errors'])
# Describe 

+-------------+---------+------------+
| name        | type    | required   |
+=============+=========+============+
| id          | integer |            |
+-------------+---------+------------+
| neighbor_id | string  |            |
+-------------+---------+------------+
| name        | string  |            |
+-------------+---------+------------+
| population  | string  |            |
+-------------+---------+------------+

# Extract 

+----+-------------+-----------+------------+
| id | neighbor_id | name      | population |
+====+=============+===========+============+
|  1 | 'Ireland'   | 'Britain' | '67'       |
+----+-------------+-----------+------------+
|  2 | '3'         | 'France'  | 'n/a'      |
+----+-------------+-----------+------------+
|  3 | '22'        | 'Germany' | '83'       |
+----+-------------+-----------+------------+
|  4 | None        | 'Italy'   | '60'       |
+----+-------------+-----------+------------+
|  5 | None        | None      | None       |
+----+-------------+-----------+------------+


# Validate 

False

# Summary 

+-----------------------------+-------------------------------------------------------------+
| Description                 | Size/Name/Count                                             |
+=============================+=============================================================+
| File name                   | /home/shashi/python/okfn/frictionless-py/data/countries.csv |
+-----------------------------+-------------------------------------------------------------+
| File size (bytes)           | 143                                                         |
+-----------------------------+-------------------------------------------------------------+
| Total Time Taken (sec)      | 0.007                                                       |
+-----------------------------+-------------------------------------------------------------+
| Total Errors                | 4                                                           |
+-----------------------------+-------------------------------------------------------------+
| Extra Cell (extra-cell)     | 1                                                           |
+-----------------------------+-------------------------------------------------------------+
| Missing Cell (missing-cell) | 3                                                           |
+-----------------------------+-------------------------------------------------------------+

# Errors 

+-------+---------+------------+----------------------------------------------------+
|   row |   field | code       | message                                            |
+=======+=========+============+====================================================+
|     4 |       5 | extra-cell | Row at position "4" has an extra value in field at |
|       |         |            | position "5"                                       |
+-------+---------+------------+----------------------------------------------------+
|     7 |       2 | missing-   | Row at position "7" has a missing cell in field    |
|       |         | cell       | "neighbor_id" at position "2"                      |
+-------+---------+------------+----------------------------------------------------+
|     7 |       3 | missing-   | Row at position "7" has a missing cell in field    |
|       |         | cell       | "name" at position "3"                             |
+-------+---------+------------+----------------------------------------------------+
|     7 |       4 | missing-   | Row at position "7" has a missing cell in field    |
|       |         | cell       | "population" at position "4"                       |
+-------+---------+------------+----------------------------------------------------+

frictionless/report/report.py Outdated Show resolved Hide resolved
* Rearranged code and placed it in the relevant places "separation of concerns"
* Made changes to tests and added new tests
* Revised summary command code
@shashigharti shashigharti force-pushed the 1095/adds-default-cli-commands branch from 3e12eb9 to 25e6ee4 Compare June 7, 2022 12:15
@shashigharti
Copy link
Contributor Author

shashigharti commented Jun 7, 2022

@roll thank you again!. I have revised the code and put in in the relevant module as you have suggested. And also changed/added tests.

I did not add to_summary function to resource because there is already a function to_view() and I used it to display the data.

I am not sure if tests > resource > validate >test_general is the right place to add the test for schema.to_summay?

Copy link
Member

@roll roll left a comment

Choose a reason for hiding this comment

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

Great!

I've added a minor comment.

Also, answering your test question -- if it's a report method tests should be in tests/report, schema method test should be in tests/schema etc

frictionless/helpers.py Outdated Show resolved Hide resolved
* replaced the report table display code with new to_summary function
* fixed failing tests
* simplified test output for multiline text
@shashigharti
Copy link
Contributor Author

Thank you @roll , I have made the changes and it is ready for the review. I also placed _wrap_text_to_colwidths function in report.py file and replaced the validate report table display in validate.py command to report.to_summary().

e936c42#diff-ca29842cf90a4bca29af564ebbb9936fce52dfb7d085a7dfe10d58124c991c65R218

what do you think of it?

frictionless/report/report.py Outdated Show resolved Hide resolved
…dation_summary as to_summary to report task class

* Changed the code of report.to_summary to use reporttask summary function
* Removed unrelated tests code added in previous commit and added new ones for schema, resource, reporttask and report
* Organized tests files for summary feature
@shashigharti shashigharti force-pushed the 1095/adds-default-cli-commands branch from a578b83 to 2b62dc1 Compare June 9, 2022 14:35
@shashigharti
Copy link
Contributor Author

Thank you @roll . I have made the changes. I have also removed the test (that I had added) that were not very related to the changes.

Copy link
Member

@roll roll left a comment

Choose a reason for hiding this comment

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

Great!

@roll roll merged commit 8ba2621 into main Jun 10, 2022
@roll roll deleted the 1095/adds-default-cli-commands branch June 10, 2022 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add default frictionless CLI command
2 participants