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

Add 19.03 release to changelog #1537

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

cakemanny
Copy link
Contributor

I've lifted the list of changes for 19.03 from the releases section on github.
The style is quite different from the 18.12 but conversely that varies a bit from the 0.8.0 section - is that a problem?

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #1537 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1537   +/-   ##
=======================================
  Coverage   91.45%   91.45%           
=======================================
  Files          18       18           
  Lines        1791     1791           
  Branches      339      339           
=======================================
  Hits         1638     1638           
  Misses        130      130           
  Partials       23       23

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 d0c8808...82f1cb6. Read the comment docs.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

I think it is fine, but I would make the issue numbers into links.

Thanks for the update 💪

@andreymal
Copy link
Contributor

Remove deleted repo from extensions list

I have a small request: make changelogs more detailed. A raw list of closed issues is a bad changelog.

Copy link
Contributor

@harshanarayana harshanarayana left a comment

Choose a reason for hiding this comment

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

This was a much-needed PR. Thanks for taking care of this.

CHANGELOG.md Outdated
19.03.1
- Fixes:
- #1529 Update project PyPI credentials
19.03.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this is not available on PyPi. So, we can merge both the change logs under 19.03.1 maybe?

@ahopkins @yunstanford @sjsadowski @vltr

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. If we really want to have it there for consistency, I would say to put everything under 19.3.1, and then have a note for 19.3.0 that says something like "skipped for package management purposes, not released on PyPI".

Copy link
Member

Choose a reason for hiding this comment

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

I hate that pypi changes the version to number based and removes leading zeroes. 🙄

Copy link
Contributor

Choose a reason for hiding this comment

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

@seemethere There's be an open issue on the setuptools for forever on this pypa/setuptools#308.

Technically on the pypi side, 19.03.1 == 19.3.1 because of PEP-440

All release segments involved in the comparison MUST be converted to a consistent length by padding shorter segments with zeros as needed.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but visually for calver 19.3.1 looks so much worse than 19.03.1 since it becomes inconsistent if you ever decide to release between October through December.

🤷‍♂️, I guess setuptools is the ultimate authority on versioning.

@cakemanny
Copy link
Contributor Author

Remove deleted repo from extensions list

I have a small request: make changelogs more detailed. A raw list of closed issues is a bad changelog.

Yes, very good point, I much agree. I'll go through these a bit later today

@cakemanny
Copy link
Contributor Author

cakemanny commented Mar 27, 2019

So, in 63179ba I've started to go through each PR and expand each of items a bit, following @andreymal's point (#1537 (comment)). It's taking a bit of time. Does it look like I'm headed in the right direction?

@harshanarayana
Copy link
Contributor

@cakemanny Yes you are in the right direction. I think some items may have been missed from your list though.

➜ git log 18.12.0..19.3 --oneline | grep '#'
abf8534 fix typo in Asyncio example (#1510)
773a66b Fix typo (#1516)
269100e format: fix linter issue causing travis build failures (fix #1514) (#1515)
2a15583 add Request.not_grouped_args, deprecation warning Request.raw_args (#1476)
d581315 Allow sanic test client to bind to a random port (#1376)
348964f Enable Middleware Support for Blueprint Groups (#1399)
e5c7589 Remove update_current_time refresh (#1502)
4260528 Fix the auto_reloader to work when the executable was launched with a module, rather than a script. (#1501)
34fe26e Add Route Resolution Benchmarking to Unit Test (#1499)
8a59907 Recognizes non-ASCII filenames in RFC 2231, and suport filename length is zero for multipart/form-data. (#1497)
52deeba Merge pull request #1490 from chenjr0719/fix_doc_build
ab56af5 Merge pull request #1489 from tomchristie/patch-1
42bf103 Remove deleted repo (#1487)
c8d2a46 did you mean specific? (#1486)
08794ae Enforce Datetime Type for Expires on Set-Cookie (#1484)
4f70dba sanic-zipkin (#1483)
b926a2c sanic#1480 Allow negative int/number in path (#1481)
52bdd1d Add stream support for bp.add_route() (#1482)
bc7d0f0 Merge pull request #1478 from chenjr0719/fix_doc_build
2758a3a Merge pull request #1472 from xxNB/dev
ef3c9ea Merge pull request #1477 from kyb3r/patch-2
9cf2e1b Merge pull request #1470 from denismakogon/create-server
99f34c9 Merge pull request #1457 from huge-success/max-age-integer
2af229e Merge pull request #1445 from huge-success/r0fls-977
8dd8e99 upgrade pytest version that compatible with pytest-cov, fixes some caplog unit tests (#1464)
96af1fe Merge pull request #1460 from huge-success/18.12-changelog
52de354 Merge pull request #1442 from Amanit/feature/gunicorn-logging
f4f90ca Merge pull request #1449 from chenjr0719/add_amending_request_object_example
cea1547 Merge pull request #1446 from huge-success/ahopkins-patch-1
fd5ae01 Merge pull request #1444 from ja8zyjits/master
19b4283 Merge pull request #1 from ja8zyjits/ja8zyjits-patch-1-readme
ff38a3c Merge pull request #1443 from huge-success/new-readme
72f2e18 Merge pull request #1440 from harshanarayana/fix/Contribution_Guide_Pip_Install
13804dc Merge pull request #1424 from harshanarayana/enh/Documentation_Update
67d51f7 Merge pull request #1423 from yunstanford/request-streaming-support

@ahopkins
Copy link
Member

I think this points at a need we should try and address: updates to the changelog during the PR, and not just after the release.

@harshanarayana
Copy link
Contributor

I think this points at a need we should try and address: updates to the changelog during the PR, and not just after the release.

Absolutely. Couldn't agree more. We can add a check in the PR to validate if the changelogs are updated as well.

@andreymal
Copy link
Contributor

In this case I have another small note: «fixed typo» changes are actually just a flood which not everyone is interested, perhaps it would be nice to sort this by importance (security fixes / new features / major bug fixes / minor fixes) or not even mention typos in the changelog because they are too minor (but it is debatable)

@cakemanny
Copy link
Contributor Author

cakemanny commented Mar 27, 2019

I don't think it's important to keep the exact format here (pyup seems to have trouble parsing I think I've noticed) but it would make sense to consider the ideas here: https://keepachangelog.com/en/1.0.0/#how. Some of them have already been touched on.

From my point of view as a user of Sanic, breaking changes are what I care most about seeing in the changelog.
To go a bit further than just say "fixed typo" changes - changes that update or add tests, and changes that update documentation aren't really of note to users. Changes to docs would ideally happen in lock-step with new features, so consumers should assume anything other bug fixes would be reflected in the docs.
I'm sure there's a developer/contributer perspective I'm overlooking here.

@ahopkins
Copy link
Member

@cakemanny Thanks for sharing the input. I think there is sort of a decision we need to make about change log and whether it should be the same as the release notes or not.

I think I can agree that there needs to be an easily digestable place for users to figure out what new features there may be, and what deprecations may be coming (without all the minor changes).

@cakemanny
Copy link
Contributor Author

Still a work in progress, but in 638192b I've gone through the rest of the PRs that were in the list I started with. I've moved the ones I don't think are significant down to their own sections, and have expanded on each of the changes and fixes that will impact consumers of the library.

- [#1470](https://github.com/huge-success/sanic/pull/1470)
Added 2 new parameters to `sanic.app.Sanic.create_server`:
- `return_asyncio_server` - whether to return an asyncio.Server.
- `asyncio_server_kwargs` - kwargs to pass to `loop.create_server` for
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really not sure I understand this this PR.
As far as I can tell the default is now just broken.
https://github.com/huge-success/sanic/pull/1470/files#diff-c4444a35dbd5e37ee480b0e8888e0880R1196
This changes the default from run_async=True to run_async=False
In which case... we need a known issues section?

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at the comment here: #1470 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That hasn't helped me I'm afraid. Sorry.

both of these programs run on 18.12 but error on 19.3:

import asyncio, sanic

loop = asyncio.new_event_loop()
app = sanic.Sanic()
server = loop.run_until_complete(app.create_server())
import sanic, uvloop

loop = uvloop.new_event_loop()
app = sanic.Sanic()
server = loop.run_until_complete(app.create_server())

I take it this is unintentional, and is a bug I should raise?

ahopkins
ahopkins previously approved these changes Mar 27, 2019
@ahopkins ahopkins dismissed their stale review March 27, 2019 11:45

Accidentally added.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

Nice work here adding some good context!

@cakemanny
Copy link
Contributor Author

I think I'm done adding proper summaries for each of the changes and fixes.
It sounds like you wanted to have a discussion on the docs, tests/dev infra, typo fixes. I haven't really expanded those, but they've been included in their own sections. Or is this good to go?

Copy link
Member

@seemethere seemethere left a comment

Choose a reason for hiding this comment

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

Would you mind squashing your commits? I think it'd be cleaner when looking back at the history. Otherwise lgtm

@harshanarayana
Copy link
Contributor

@cakemanny 👍 Great job. Thanks for taking care of this.

@harshanarayana
Copy link
Contributor

harshanarayana commented Mar 28, 2019

It sounds like you wanted to have a discussion on the docs, tests/dev infra, typo fixes.

I am creating a new thread of the community forum for the same so that we can continue the discussion there without breaking the chain here in this PR.

EDIT

Community thread

@cakemanny cakemanny force-pushed the update-changelog-19.03 branch from d16fa22 to 82f1cb6 Compare March 28, 2019 11:00
@cakemanny
Copy link
Contributor Author

Sure, no problem. Squashed :)

Copy link
Contributor

@harshanarayana harshanarayana left a comment

Choose a reason for hiding this comment

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

LGTM

@ahopkins ahopkins dismissed seemethere’s stale review March 28, 2019 18:47

Commits squashed

@sjsadowski sjsadowski merged commit 3bedb22 into sanic-org:master Mar 29, 2019
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.

6 participants