Skip to content

Conversation

@ayan-b
Copy link
Member

@ayan-b ayan-b commented Apr 21, 2019

Before raising the PR, here is a check list:

  1. have you written unit tests for your code changes?
  2. have you run "make format"?
  3. are you requesting to "dev"?
  4. have you updated the change log?
  5. do you think that you can understand your changes after 6 month?
    5.1) can someone else understand your changes without your explanation?
  6. are you pround of your code changes?
    6.1) do you have the feeling of achievement?
  7. please add your name and github link to contributors.rst in alphabetical order.

@ayan-b ayan-b force-pushed the encoding branch 2 times, most recently from 6636349 to eb8589c Compare April 21, 2019 14:44
@codecov-io
Copy link

codecov-io commented Apr 21, 2019

Codecov Report

Merging #267 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #267      +/-   ##
==========================================
+ Coverage   99.19%   99.19%   +<.01%     
==========================================
  Files          58       59       +1     
  Lines        2478     2496      +18     
==========================================
+ Hits         2458     2476      +18     
  Misses         20       20
Impacted Files Coverage Δ
moban/copy/__init__.py 100% <100%> (ø) ⬆️
tests/test_copy_encoding.py 100% <100%> (ø)
moban/utils.py 97.7% <100%> (+0.05%) ⬆️
tests/test_copy_engine.py 100% <100%> (ø) ⬆️

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 9452524...e4442d0. Read the comment docs.

@ayan-b ayan-b changed the title Encoding Use simple open in copy engine Apr 21, 2019
@chfw
Copy link
Member

chfw commented Apr 21, 2019

Utils.py does the file writing, in 'wb' mode.

@chfw
Copy link
Member

chfw commented Apr 21, 2019

Python 3 failed but python 2 tests passed.

@chfw
Copy link
Member

chfw commented Apr 21, 2019

def write_file_out(filename, content, strip=True, encode=True):

@ayan-b
Copy link
Member Author

ayan-b commented Apr 21, 2019

Python 3 failed but python 2 tests passed.

Pretty strange, I will look into it. Seems something to do with encoding.

@chfw
Copy link
Member

chfw commented Apr 21, 2019

Do you have the file which triggered this PR? Can we have a test case using it and get it pass?

Please put it in a separate test file because it will serve us in the future as critical regression test suites.

@ayan-b
Copy link
Member Author

ayan-b commented Apr 21, 2019

@chfw
Copy link
Member

chfw commented Apr 21, 2019

I see

@chfw
Copy link
Member

chfw commented Apr 21, 2019

Please Write up a new test case to copy it and verify copy works ok.

And this test case will not be updated when moban evolves because it will be the future that we do like to break.

@ayan-b
Copy link
Member Author

ayan-b commented Apr 21, 2019

@chfw, I have added the test in an existing file. Should I create a new file and add there?

@ayan-b ayan-b self-assigned this Apr 21, 2019
@chfw
Copy link
Member

chfw commented Apr 22, 2019

A new file is better because it is to be kept as it is during refactoring. Other test cases that fail in refactoring can be updated but this one cannot be.

So if this test fails, it means we got a regression.

@ayan-b
Copy link
Member Author

ayan-b commented Apr 22, 2019

@chfw is the current structure ok?

Copy link
Member

@chfw chfw left a comment

Choose a reason for hiding this comment

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

LGTM

@chfw chfw merged commit 5d1674a into moremoban:dev Apr 22, 2019
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.

3 participants