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.

Sign up for GitHub

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

Jump to bottom

Move support to OTP 22+ #114

Closed
wants to merge 7 commits into from
Closed

Move support to OTP 22+ #114

wants to merge 7 commits into from

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Jul 27, 2023 •
edited
Loading

Closes #103.

This pull request is being split into several other pull requests, to ease review. These are listed below.

paulo-ferraz-oliveira
paulo-ferraz-oliveira commented Jul 27, 2023
View reviewed changes
elvis.config Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Work in blacklist mode (accept all defaults except...), and add test to the mix.

paulo-ferraz-oliveira
paulo-ferraz-oliveira commented Jul 27, 2023
View reviewed changes
src/elli_http.erl
Comment on lines -61 to +63
case catch elli_tcp:accept(ListenSocket, Server, accept_timeout(Options)) of
case elli_tcp:accept(ListenSocket, Server, accept_timeout(Options)) of
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 don't see where catch was actually used, since all the clauses derive not from exceptions, but simple function returns.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023 •
edited
Loading

Codecov Report

Patch coverage: 82.35% and project coverage change: +0.07% 🎉

Comparison is base (3ec3522) 76.32% compared to head (829beda) 76.40%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   76.32%   76.40%   +0.07%     
==========================================
  Files          12       12              
  Lines         756      750       -6     
==========================================
- Hits          577      573       -4     
+ Misses        179      177       -2
Files Changed Coverage Δ
src/elli_middleware.erl 100.00% <ø> (ø)
src/elli_sendfile.erl 62.50% <ø> (ø)
src/elli_tcp.erl 86.66% <ø> (ø)
src/elli_util.erl 100.00% <ø> (ø)
src/elli_request.erl 78.49% <50.00%> (ø)
src/elli_http.erl 68.13% <80.00%> (+0.02%) ⬆️
src/elli.erl 77.61% <100.00%> (ø)
src/elli_example_callback.erl 90.62% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 27, 2023 •
edited
Loading

CI failing for OTP 26, 24, and 22. I'll try to replicate locally.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 27, 2023 •
edited
Loading

It's possible the failure for non-OTP 26 is due to #99, since re-executing the tests (in a fork) showed ✅.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 28, 2023 •
edited
Loading

Removed OTP 26 from scope, since there're SSL -related issues to solve. Should be moved to a new PR, I guess...

@tsloughter
Copy link
Member

Woo, these are all looking good.

And yea, for 26 the ssl certs may need to be re-created. I had to do this for chatterbox tests too.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Is anything lacking for this one to get merged? I can then rebase and update the other ones.

I think I'll deal with OTP 26 in a separate PR so as to not inflict further confusion in these pull requests... 😄

This was referenced Aug 2, 2023
Merged
Merged
Open
@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter, if you prefer I can also reduce the scope for this and do multiple pull requests to an isolated branch until you're fully satisfied and then we merge to the main branch.

@tsloughter
Copy link
Member

This doesn't look too bad, I just hadn't taken a close look.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter, what's required to push this forward?

@tsloughter
Copy link
Member

Hey sorry, it is just a large PR. But it looks like a lot of it is formatting?

@paulo-ferraz-oliveira
Copy link
Contributor Author

👋 it's probably the commit related to Elvis, but I can try to break it down further.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft June 8, 2024 16:18
@paulo-ferraz-oliveira
Copy link
Contributor Author

I'm moving to draft, to be able to break it down into several pull requests. Once that's done, I'll close this one without it being merged.

@paulo-ferraz-oliveira paulo-ferraz-oliveira mentioned this pull request Jun 8, 2024
Merged
@paulo-ferraz-oliveira paulo-ferraz-oliveira mentioned this pull request Jun 8, 2024
Merged
@paulo-ferraz-oliveira
Copy link
Contributor Author

All new pull requests mentioned in the top description.

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/otp-21+ branch June 8, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop OTP =<21 support
3 participants
@paulo-ferraz-oliveira @codecov-commenter @tsloughter