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

[ISSUE-68] Improved Specs and Added Coverage Report #70

Merged
merged 17 commits into from
Jul 19, 2024
Merged

[ISSUE-68] Improved Specs and Added Coverage Report #70

merged 17 commits into from
Jul 19, 2024

Conversation

jlurena
Copy link
Collaborator

@jlurena jlurena commented Jul 9, 2024 •
edited
Loading

Summary

  • Removed support for Ruby < 3.0
  • Removed support for Rails < 6.1
  • Added Coverage Report to Github CI
  • Added Linter to Github CI
  • Updated RSpec to use latest syntax and updated specs
  • Updated to a breaking version, 9.0

@mhgbrown mhgbrown mentioned this pull request Jul 9, 2024
Closed
@jlurena jlurena requested a review from mhgbrown July 9, 2024 17:03
@jlurena jlurena marked this pull request as draft July 9, 2024 17:04
@jlurena
Copy link
Collaborator Author

jlurena commented Jul 9, 2024 •
edited
Loading

@mhgbrown Any reason we are maintaining ruby < 3.0?

https://endoflife.date/ruby

Everything under 3.0 hit EOL

Edit: @mhgbrown if you'd consider this I also did the following:

  • Removed support for Ruby < 3.0
  • Removed support for Rails < 6.1
  • Added Coverage Report to Github CI
  • Added Linter to Github CI
  • Updated RSpec to use latest syntax and updated specs
  • Updated to a breaking version, 9.0

@jlurena jlurena force-pushed the ISSUE-68 branch 3 times, most recently from 1c1c471 to b2e08ed Compare July 9, 2024 20:57
@jlurena jlurena force-pushed the ISSUE-68 branch 17 times, most recently from 85a7efb to dee8057 Compare July 9, 2024 22:33
@jlurena jlurena marked this pull request as ready for review July 9, 2024 22:38
@mhgbrown
Copy link
Owner

No particular reason other than to be most widely compatible. I'm down to move forward and support versions that are "living".

mhgbrown
mhgbrown reviewed Jul 11, 2024
View reviewed changes
Copy link
Owner

@mhgbrown mhgbrown left a comment

Choose a reason for hiding this comment

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

This is great stuff and really moves the project forward. I left some questions for you.

.github/workflows/ruby.yml Outdated Show resolved Hide resolved
.github/workflows/ruby.yml Outdated Show resolved Hide resolved
.github/workflows/ruby.yml Outdated Show resolved Hide resolved
spec/support/matchers.rb Show resolved Hide resolved
@jlurena
Copy link
Collaborator Author

jlurena commented Jul 11, 2024

This is great stuff and really moves the project forward. I left some questions for you.

It is done. Please review again. @mhgbrown

@jlurena
Copy link
Collaborator Author

jlurena commented Jul 15, 2024

This is great stuff and really moves the project forward. I left some questions for you.

It is done. Please review again. @mhgbrown

@mhgbrown I can merge but I'd need you to publish 😄

@mhgbrown
Copy link
Owner

mhgbrown commented Jul 15, 2024 •
edited
Loading

I'll get on it later today

@jlurena
[ISSUE-68] - Removed concurrency option
c26b44b 
It does more bad than good in real world scenarios. Performance degrades.
@jlurena
Copy link
Collaborator Author

jlurena commented Jul 18, 2024

I'll get on it later today

@mhgbrown 👀

@mhgbrown
Copy link
Owner

mhgbrown commented Jul 18, 2024 •
edited
Loading

@jlurena Lawd, sorry. This week suddenly became a week of interviewing/doing a trial at a company. Why remove the concurrency option?

@jlurena
Copy link
Collaborator Author

jlurena commented Jul 19, 2024

@jlurena Lawd, sorry. This week suddenly became a week of interviewing/doing a trial at a company. Why remove the concurrency option?

After research and practical tests, it actually performed really bad. Tested with a redis cache and a memcache cache. Performance degraded significantly. It was visibly slower. @italijancic-th helped test

mhgbrown
mhgbrown approved these changes Jul 19, 2024
View reviewed changes
Copy link
Owner

@mhgbrown mhgbrown left a comment

Choose a reason for hiding this comment

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

Looks great!

@mhgbrown
Copy link
Owner

ok got it! I'll merge it in!

@mhgbrown mhgbrown merged commit 9c3a180 into mhgbrown:master Jul 19, 2024
12 checks passed
@mhgbrown
Copy link
Owner

@jlurena finally released: https://rubygems.org/gems/cached_resource

@jlurena jlurena deleted the ISSUE-68 branch July 22, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhgbrown mhgbrown mhgbrown approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jlurena @mhgbrown @italijancic-th