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

Fix test + remove buffer options + return nb pages #10

Closed
wants to merge 15 commits into from
Closed

Fix test + remove buffer options + return nb pages #10

wants to merge 15 commits into from

Conversation

Atinux
Copy link
Contributor

@Atinux Atinux commented Jan 14, 2015

Hello!

Thanks for your awesome node module!
I removed the buffer option in this version (already deprecated in
0.3.0) + the script return a JSON with the number of pages in the PDF!

I also updated the README and the documentation!

I you like it, don’t forget to npm publish :)

Regards,
Sebastien

Hello!

Thanks for your awesome node module!
I removed the buffer option in this version (already deprecated in
0.3.0) + the script return a JSON with the number of pages in the PDF!

I also updated the README and the documentation!

I you like it, don’t forget to `npm publish` :)

Regards,
Sebastien
@marcbachmann
Copy link
Owner

Thank you for your contribution.
Sorry, I haven't set up appveyor or travis correctly. I'll have a detailed look into your pull-request somewhen this week.

@Atinux
Copy link
Contributor Author

Atinux commented Jan 14, 2015

Thank you :)

@Atinux
Copy link
Contributor Author

Atinux commented Jan 14, 2015

I added the tests for Travis and AppVeyor too!

@@ -1,3 +1,8 @@
0.4.0
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest you to do version bumps always in the master branch after a feature is merged. It looks strange if there are that many other commits after creating a changelog with a specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right!

@marcbachmann
Copy link
Owner

I'd like to have the pages in the response. But sadly this only works if we define a header or footer because we only execute that method when rendering headers or footers. That might look buggy.
What do you think about that?

The travis config is also ok to merge. Can you create a new pull-request only with the travis file? I can merge that directly. I'll implement appveyor separately (for windows tests).

With the other things i'd like to wait because I've changed the api a bit.
I hope I didn't discourage you with my many comments and requests. Thanks again for your work.

@Atinux
Copy link
Contributor Author

Atinux commented Jan 15, 2015

I agree, this is annoying that we cannot have the right number of pages without a header or footer given.

I think you can forget this pull request if you changed the API, just maybe be inspired for futur changes by what I've done.

:)

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

2 participants