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

Implement the correct method in the server_connection #78

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

sacovo
Copy link
Contributor

@sacovo sacovo commented Mar 23, 2024

The first implementation used upgrade, which was then changed to upgrade_protocol, but the change wasn't reflected in the implementation of the session used by the actual http server.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 23, 2024
@SeanTAllen
Copy link
Member

@mfelsche @sacovo what is the bug that this fixes?

@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Mar 26, 2024
@ponylang-main
Copy link
Contributor

Hi @sacovo,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 78.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen SeanTAllen added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge labels Mar 26, 2024
@SeanTAllen
Copy link
Member

So, there's a default method on the Session trait. Which means that the incorrectly named upgrade instead of upgrade_protocol on _server_connection isn't caught. That's the fix here and should be indicated as such in the release notes.

Copy link
Contributor

@mfelsche mfelsche left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @sacovo !

If this gets some release notes, this is good to go.

.release-notes/78.md Outdated Show resolved Hide resolved
.release-notes/78.md Outdated Show resolved Hide resolved
@SeanTAllen SeanTAllen merged commit 2ae18a0 into ponylang:main Apr 2, 2024
4 checks passed
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Apr 2, 2024
github-actions bot pushed a commit that referenced this pull request Apr 2, 2024
github-actions bot pushed a commit that referenced this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants