-
Notifications
You must be signed in to change notification settings - Fork 31
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
Replace CHANGELOG.md with NEWS.md similar to other shiny.tools packages #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request is totally wrong.
- Items are assigned to wrong releases. Example:
Fixed width for small screens (mobile) - has been assigned to shiny.router 0.2.0. The PR with this change has been merged on Dec 8, 2016. The first release of shiny.router happened on 2018-09-18... - Wrong version numbers - first release was 0.1.1, not 0.1.0, missing 0.2.2. We are not using "latest" to mark the current release.
- Wrong choice of items. The changelog should include only code changes, not documentation or new demo. E.g. for 0.3.0, the only item that should be there is the new API.
- Some items are not described well enough, e.g. new API introduced in version 0.3.0 is a big change that deserves more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task requires not only copy-pasting Pull Requests titles, but also checking, what was actually changed. I pointed some errors, but you need to go through most of the items to make sure, if the description is correct.
Also, this PR should add the changelog to the documentation page.
NEWS.md
Outdated
- Added a new API that is compatible with Rhino | ||
- `router_ui` and `router_server` replaced `make_router` | ||
- `router_ui` should be applied inside the UI function (not outside, like `make_router`) and thanks to that, can utilize `ns()` if used in a Rhino app | ||
- `router_server` adds all required server components (mostly `observeEvents`) to the server part of the application | ||
- Marked `make_router` as deprecated, it is not removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It suggests that introducing
router_ui
androuter_server
is something different than the new API. This has to be a single point. Don't add so many low-level technical details. It should include information that there is a new API compatible with Rhino (you can mention function names) and the old one is deprecated.
NEWS.md
Outdated
|
||
# shiny.router 0.2.0 | ||
|
||
- Introduced a new version of the router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check what is happening in the PRs, not only check the titles! For example, this one is just a preparation for the CRAN release - polishing documentation, updating the page, etc.)
NEWS.md
Outdated
|
||
# shiny.router 0.2.3 | ||
|
||
- Applied `shiny::createWebDependency()` before `renderDependencies()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good description - this informs how the bug was solved, but tells nothing about what the bug was. Again, you need to read the PRs - in this particular case, you would find the link to the issue with bug description. Based on that, create an entry for the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please resolve the comment and you can merge. Please use "squash and merge" option to avoid flooding the git history with unnecessary commits.
Add NEWS.md file