Skip to content

Conversation

@ayan-b
Copy link
Member

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

Closes #265

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 requested a review from chfw April 27, 2019 14:19
@ayan-b ayan-b self-assigned this Apr 27, 2019
@codecov-io
Copy link

codecov-io commented Apr 27, 2019

Codecov Report

Merging #268 into dev will decrease coverage by 0.07%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #268      +/-   ##
==========================================
- Coverage   99.19%   99.12%   -0.08%     
==========================================
  Files          59       59              
  Lines        2496     2505       +9     
==========================================
+ Hits         2476     2483       +7     
- Misses         20       22       +2
Impacted Files Coverage Δ
moban/plugins/template.py 97.82% <100%> (+0.01%) ⬆️
tests/test_copy_encoding.py 100% <100%> (ø) ⬆️
moban/utils.py 95.6% <60%> (-2.1%) ⬇️

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 5d1674a...98c85e4. Read the comment docs.

@ayan-b ayan-b changed the title Ignore encode error if occurs [WIP] Ignore encode error if occurs Apr 27, 2019
@ayan-b
Copy link
Member Author

ayan-b commented Apr 27, 2019

Job passes: https://gitlab.com/ayan-b/mobans/-/jobs/203626328. Adding & updating tests due.

@chfw
Copy link
Member

chfw commented Apr 27, 2019

I see now. It is because of replacing function, we need to decide the content, for copy engine, it is not needed. So it means current architecture is at fault, meaning it is not general enough to accommodate template and copy engine.

@ayan-b
Copy link
Member Author

ayan-b commented Apr 27, 2019

What about this #265 (comment)? I think previously we used to do this.

@chfw
Copy link
Member

chfw commented Apr 27, 2019

By the way, without reading the content, hashstore may not function well. So os copy will not work well.

In general, the same content will not be templates/copied twice.

@ayan-b ayan-b changed the title [WIP] Ignore encode error if occurs Ignore encode error if occurs Apr 28, 2019
@ayan-b
Copy link
Member Author

ayan-b commented Apr 28, 2019

@chfw please review.

@chfw
Copy link
Member

chfw commented Apr 28, 2019

the test case should trigger 'copy engine' end-to-end, meaning: a file is copied to output file path. At the moment, it does not do it.

@chfw
Copy link
Member

chfw commented Apr 28, 2019

I was on holiday. I should be able to help next week.

@ayan-b
Copy link
Member Author

ayan-b commented Apr 30, 2019

Maybe we can pass a mobanfile in the handle_mobanfile function and check the output?
I will try to solve this once my exams are over.

@chfw
Copy link
Member

chfw commented Apr 30, 2019

no hurry!

@ayan-b
Copy link
Member Author

ayan-b commented May 13, 2019

the test case should trigger 'copy engine' end-to-end, meaning: a file is copied to output file path. At the moment, it does not do it.

Can you please help me here?

@chfw
Copy link
Member

chfw commented May 20, 2019

let me find some time. I was thinking of end2end test like what we did for all documentations.

like this: https://github.com/moremoban/moban/tree/dev/docs/level-15-copy-templates-as-target

@chfw
Copy link
Member

chfw commented May 20, 2019

could you please try merge it with https://github.com/moremoban/moban/tree/regression-test-coping-binary-file?

@chfw chfw changed the base branch from dev to regression-test-coping-binary-file May 20, 2019 17:40
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.

let's see what happens on the feature branch

@chfw chfw merged commit ab9f52c into moremoban:regression-test-coping-binary-file May 20, 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