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

Feature/130 report cleanup #138

Closed
wants to merge 2 commits into from
Closed

Conversation

nfarabullini
Copy link
Contributor

@nfarabullini nfarabullini commented Jun 19, 2019

Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

@nfarabullini, I think we should not rush things and somehow discuss / plan changes better.

The current status of the branch does not quite address the points in #130, which were in fact just mentioning some preliminary, non-exhaustive examples. Some of the changes even go in the wrong direction.

There should be least some minimal discussion / common understanding about existing issues. Quick implementations with sudden PR, without any sort of discussion or good understanding of what the issue is about is definitely not the way to go.

I am in fact going to close this PR, we should rather go back to the issue first, and re-open a new PR at a later point. We shall also consider reverting the changes on the branch, possibly cherry-picking the A4 format commit e1438f1 in a separate branch dedicated to #131 only (happy to help with and show what this is about, TL;DR: git cherry-pick e1438f16493039132c28bdea094921fc6fed9cb3 )

@riccardoporreca riccardoporreca deleted the feature/130-report-cleanup branch October 1, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Technical review of R Markdown PDF report A4 PDF report
2 participants