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 Python 3.8 build #48

Closed
wants to merge 1 commit into from

Conversation

irisliucy
Copy link
Contributor

@irisliucy irisliucy commented May 21, 2020

The current issue #45 is caused by inconsistent keys in tabularInput with csvOutput headers. To fix this:

  1. Read the existing log file
  2. Update the csv Dictwriter with the new header using union
  • Union of tabularInput's and csvOutput's header is used to make sure all keys from both instances are captured
  1. Write the same log file with new key headers and old data. If the value of new key is missing, the cell is left blank.

The cons of this solution:

  • The csv log file is read and write to update the inconsistent key headers. It may be time-consuming to handle a large number of csv log files because of the heavy I/O. To ensure the I/O is as efficient as possible, all file reading and writing is done in RAM and file is closed upon used.

A better approach can be:

  • Expand the header on-the-go as a new key(s) is encountered. Write the log file with the new header exactly once.

#45
@avnishn
@zequnyu

@irisliucy irisliucy requested a review from a team as a code owner May 21, 2020 04:39
@irisliucy irisliucy requested a review from ryanjulian May 21, 2020 04:39
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #48 into master will decrease coverage by 0.83%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   94.13%   93.29%   -0.84%     
==========================================
  Files           7        7              
  Lines         358      373      +15     
  Branches       58       60       +2     
==========================================
+ Hits          337      348      +11     
- Misses         12       16       +4     
  Partials        9        9              
Impacted Files Coverage Δ
src/dowel/csv_output.py 90.56% <73.33%> (-6.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8b5134...5351f6e. Read the comment docs.

@ryanjulian ryanjulian closed this May 21, 2020
@avnishn
Copy link
Member

avnishn commented May 21, 2020

@irisliucy you need to open this pr onto your fork and then email us, as per the instructions.

@irisliucy
Copy link
Contributor Author

@avnishn Sorry for the inconvenience. I've opened this pr to my fork repo instead. I appreciated your reminder.

@ryanjulian ryanjulian changed the title Fix #47(Inconsistent tabularInput Keys + test) Add Python 3.8 build May 21, 2020
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.

None yet

3 participants