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

Drop RequestStore in favour of ActiveSupport::CurrentAttributes #1447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshuay03
Copy link

@joshuay03 joshuay03 commented Nov 16, 2023

Thank you for your contribution!

Check the following boxes:

  • Wrote good commit messages.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

Fixes #1444

I wonder if there are any performance benefits here, I'm happy to run some benchmarks if it's a valid concern.

@joshuay03 joshuay03 force-pushed the drop-request-store-for-active-support-current-attributes branch from 9b758ad to 1f76b9f Compare November 16, 2023 09:59
@@ -5,8 +5,6 @@
RSpec.describe WidgetsController, type: :controller, versioning: true do
before { request.env["REMOTE_ADDR"] = "127.0.0.1" }

after { RequestStore.store[:paper_trail] = nil }
Copy link
Author

Choose a reason for hiding this comment

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

Current attributes are automatically reset before each request so this is no longer necessary for the new implementation.

@joshuay03 joshuay03 force-pushed the drop-request-store-for-active-support-current-attributes branch from 1f76b9f to d363b54 Compare November 17, 2023 08:12
@joshuay03 joshuay03 force-pushed the drop-request-store-for-active-support-current-attributes branch 5 times, most recently from 9d977a8 to 8c58e67 Compare November 17, 2023 11:48
@joshuay03
Copy link
Author

CI is failing cause of reduced line coverage, but all my changes except for the new CurrentAttributes class (which I've added some coverage for) are already covered in existing tests...

@joshuay03 joshuay03 force-pushed the drop-request-store-for-active-support-current-attributes branch from 8c58e67 to c43279b Compare November 20, 2023 00:11
Copy link

This PR has been automatically marked as stale due to inactivity.
The resources of our volunteers are limited.
If this is something you are committed to continue working on, please address any concerns raised by review and/or ping us again.
Thank you for all your contributions.

@github-actions github-actions bot added the Stale label Feb 22, 2024
@github-actions github-actions bot closed this Mar 1, 2024
@ngan
Copy link

ngan commented May 1, 2024

@jaredbeck would you mind taking a look at this? 🙏

@skipkayhil
Copy link

Re-opening because it was closed as stale but I do think its a good idea

@skipkayhil skipkayhil reopened this Nov 27, 2024
@github-actions github-actions bot removed the Stale label Nov 28, 2024
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.

Drop RequestStore in favor of ActiveSupport::CurrentAttributes
3 participants