Skip to content

Conversation

@CLiu13
Copy link
Collaborator

@CLiu13 CLiu13 commented Nov 15, 2018

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?

@CLiu13
Copy link
Collaborator Author

CLiu13 commented Nov 15, 2018

I read through the changelog and cannot find any entry describing the addition of handlebars as an engine option - would you also like me to add that to the changelog?

@codecov-io
Copy link

codecov-io commented Nov 15, 2018

Codecov Report

Merging #128 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #128   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          39       39           
  Lines        1921     1921           
=======================================
  Hits         1894     1894           
  Misses         27       27

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 9718f5e...16cce07. Read the comment docs.

@PrajwalM2212
Copy link
Collaborator

PrajwalM2212 commented Nov 15, 2018

@CLiu13 We do not intend to include handlebars in the main repo. Infact we will be moving it to a separate repo. So it can't be a additional feature to main repo.

@chfw
Copy link
Member

chfw commented Nov 15, 2018

@CLiu13 , the change log looks OK. please do this:

  1. update it in .moban.cd/changelog.yml
  2. then call make update, which auto generates changelog.rst

Otherwise, next make update will overwrite your change log.

I know it is new piece of information. Please feel free to update docs.

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.

please update it in changelog.yml instead.

@CLiu13
Copy link
Collaborator Author

CLiu13 commented Nov 15, 2018

Thanks for the info on the correct way to update the changelog. Running make update resulted in some additional changes in other files, which I have included for now - but let me know if you would like me to exclude them.

EDIT: One of those changes resulted in failing travis tests - as a result, I will exclude that change.

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.

good to go

@chfw chfw merged commit 6de80f2 into moremoban:dev Nov 15, 2018
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.

4 participants