Develop issue91: National request for table size change #96
Conversation
…t when it is long.
…nts from biology group.
xiaoliz0
changed the title
marrip
left a comment
marrip
left a comment
There was a problem hiding this comment.
Nice work! I still see some potential for improvement. Please let me know what you think. I can also come up with some suggestions if you want
Script/PRONTO.py
Outdated
| if_print_rowNo = True | ||
| table8_column_width = [0.54, 0.96, 0.96, 0.51, 0.73, 1.12, 2.26, 0.79, 0.81, 0.53] | ||
| slide8_table_nrows = insert_table_to_ppt(slide8_table_data_file,slide8_table_ppSlide,slide8_table_name,slide8_header_left,slide8_header_top,slide8_header_width,slide8_table_left,slide8_table_top,slide8_table_width,slide8_table_height,slide8_table_font_size,slide8_table_header,output_ppt_file,if_print_rowNo,table8_column_width) | ||
| table_max_rows_per_slide = int(cfg.get("INPUT", "table_max_rows_per_slide")) - 1 |
There was a problem hiding this comment.
why do we substract 1 here? Is it because of the header?
There was a problem hiding this comment.
Yes, the header will take 1 row for each slide.
There was a problem hiding this comment.
should we then just supply the correct number directly instead of reading it and changing it immediately? Would that make more sense?
There was a problem hiding this comment.
Sounds good. I will update then.
Script/PRONTO.py
Outdated
| slides_list = list(slides) | ||
| slides.remove(slides_list[7]) | ||
| slides.insert(12,slides_list[7]) | ||
| slides.insert(slide_count + 1,slides_list[7]) |
There was a problem hiding this comment.
Emm, this slide is the last slide in the template file, and need to be still the last slide of the final report. Since it was removed, so it will be added to be as one more slide. I could update the function to make it more easy to understand. :)
Script/PRONTO.py
Outdated
| return data_nrows | ||
|
|
||
|
|
||
| def insert_table_to_ppt_end(table_data_file,slide_n,table_name,left_h,top_h,width_h,left_t,top_t,width_t,height_t,font_size,table_header,output_ppt_file,if_print_rowNo,table_column_width,table_max_rows_per_slide): |
There was a problem hiding this comment.
I see that the function is similar to insert_table_to_ppt - my first question would be, is it possible to combine them and turn on the max rows per slide specifically when needed instead of having similar business logic in two functions - another option would be to refactor the original function and reuse the blocks in both functions that are reusable. Let me know what you think.
I also feel like there are so many input variables, which makes it hard to see all the different values that go in - would it make sense to maybe group some of them into dicts? It might make sense to define a class and add some helper functions to it?
Finally, the function should have unit tests as we have discussed in the last workshop. Would it be possible to add different test cases to make sure it always works as expected?
There was a problem hiding this comment.
I have merged the 2 functions into 1 now. Feel free to add comments.
… inserting table into one function.
marrip
left a comment
marrip
left a comment
There was a problem hiding this comment.
very nice work. That looks much better, indeed! I was wondering if we could also refactor it a bit by breaking it into smaller functions and adding unit testing for those? If you want, I can come up with something if you don't have time to take a look.
It is nice if you could give something as examples. I am not sure which way will be better for this issue. Thanks in advance! |
…e in the end of the report, sinice it is not in use in any other places.
…to develop_issue91
National request: Adjust tables in report so long tables are split over multiple slides.