Skip to content

Conversation

@ayan-b
Copy link
Member

@ayan-b ayan-b commented Mar 1, 2019

Closes #234

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 changed the title Expand the definition of template_type in targets section [WIP] Expand the definition of template_type in targets section Mar 1, 2019
@codecov-io
Copy link

codecov-io commented Mar 1, 2019

Codecov Report

Merging #245 into dev will decrease coverage by 0.03%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #245      +/-   ##
=========================================
- Coverage   99.33%   99.3%   -0.04%     
=========================================
  Files          57      57              
  Lines        2402    2435      +33     
=========================================
+ Hits         2386    2418      +32     
- Misses         16      17       +1
Impacted Files Coverage Δ
moban/jinja2/engine.py 100% <ø> (ø) ⬆️
moban/reporter.py 100% <100%> (ø) ⬆️
moban/_version.py 100% <100%> (ø) ⬆️
tests/mobanfile/test_targets.py 100% <100%> (ø) ⬆️
tests/test_docs.py 100% <100%> (ø) ⬆️
tests/test_reporter.py 100% <100%> (ø) ⬆️
moban/mobanfile/targets.py 98.3% <92.85%> (-1.7%) ⬇️

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 00b6c77...b5d35bb. Read the comment docs.

@ayan-b ayan-b changed the title [WIP] Expand the definition of template_type in targets section Expand the definition of template_type in targets section Mar 1, 2019
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.

I am sorry that My bad has lead to 'overrides'. Please use 'base_type' so that there is a consistency in our design.

@ayan-b
Copy link
Member Author

ayan-b commented Mar 2, 2019

Sure!

@chfw
Copy link
Member

chfw commented Mar 2, 2019

It is great that you have finished 1st round of iteration: code changed, tests written. I am now going to give a bit more comment on code refactoring side, while keep tests in-tact.

Please be aware that: it is normal to do code refactoring after a working prototype is done. some developer take a defence on whatever is written. I hope we can overcome our natural instincts.

@ayan-b ayan-b force-pushed the cusvar branch 2 times, most recently from d7dd17d to 82ab306 Compare March 2, 2019 13:47
@ayan-b ayan-b requested a review from chfw March 2, 2019 13:49
@ayan-b
Copy link
Member Author

ayan-b commented Mar 2, 2019

If I run moban inside docs/level-18-..., I am getting the following result:

Templating a.template.file_type_of_my_choice to a.output
Templated 1 file.
Templating a.template.jj2 to b.output
Templated 1 file.

Looking into it.

@ayan-b ayan-b self-assigned this Mar 2, 2019
@chfw
Copy link
Member

chfw commented Mar 2, 2019

It means your moban has not been updated. Install it again.

@chfw
Copy link
Member

chfw commented Mar 2, 2019

since you are the first one after 0.4.1, please pump up the version, which also helps you know which version was installed.

@ayan-b ayan-b requested a review from chfw March 2, 2019 20:12
@ayan-b
Copy link
Member Author

ayan-b commented Mar 2, 2019

@chfw please review. 😄

@ayan-b ayan-b requested a review from chfw March 3, 2019 09:29
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.

Almost there. Minor issues now: reporter will print out messages so no need to print again.

"file_extensions": [file_extension], could use the constant.

@ayan-b ayan-b requested a review from chfw March 3, 2019 10:49
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.

Great! Will merge once I am near to a computer.

@chfw
Copy link
Member

chfw commented Mar 3, 2019

To test console print , you can mock out sys.stdout. If you search within this repo, there may be some existing code.

@chfw
Copy link
Member

chfw commented Mar 3, 2019

Yep. If you use 'with', you won't need to do patcher start and stop.

@chfw chfw merged commit 5133306 into moremoban:dev Mar 3, 2019
@chfw
Copy link
Member

chfw commented Mar 3, 2019

have squash and merged. for next item in line, please sync with dev first and branch off from there.

@ayan-b
Copy link
Member Author

ayan-b commented Mar 3, 2019

I guess it was already synced with Dev, else GitHub would have shown Update branch.
And I was about push another commit and then squash it altogether. 😅

@ayan-b ayan-b deleted the cusvar branch March 3, 2019 14:11
@chfw
Copy link
Member

chfw commented Mar 3, 2019

hoho.. sorry. please do it with another PR then.

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