-
Notifications
You must be signed in to change notification settings - Fork 148
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
Conversation
* Added command named summary * Added to_summary function to ReportTask * Added tests for both 'summary' command and 'to_summary' function
* Rearranged code and placed it in the relevant places "separation of concerns" * Made changes to tests and added new tests * Revised summary command code
3e12eb9
to
25e6ee4
Compare
@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? |
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.
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
* replaced the report table display code with new to_summary function * fixed failing tests
* simplified test output for multiline text
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? |
…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
a578b83
to
2b62dc1
Compare
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. |
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.
Great!
fixes Add default
frictionless
CLI command #1095Shall I also update the documentation here
https://framework.frictionlessdata.io/docs/guides/validation-guide/#validation-errors to add "to_summary" info.