Skip to content

Clarify Moneta integration in Circuit Store documentation#203

Merged
matthewshafer merged 1 commit intoyammer:mainfrom
neilvcarvalho:patch-1
May 22, 2025
Merged

Clarify Moneta integration in Circuit Store documentation#203
matthewshafer merged 1 commit intoyammer:mainfrom
neilvcarvalho:patch-1

Conversation

@neilvcarvalho
Copy link
Contributor

I found the Circuit Store documentation about Moneta integration confusing, particularly the statement that Moneta "needs to be loaded prior to use." After testing, I confirmed that there's no specific loading order requirement between Circuitbox and Moneta.

This commit updates the documentation to:

  • Remove potentially confusing language about loading requirements
  • Explicitly state that Circuitbox uses an in-memory store by default
  • Present Moneta as an optional alternative storage option
  • Preserve the existing Moneta compatibility requirements

These changes should help other developers better understand how to integrate Moneta with Circuitbox if they choose to use an alternative store.

@neilvcarvalho
Copy link
Contributor Author

@microsoft-github-policy-service agree

@matthewshafer
Copy link
Collaborator

After testing, I confirmed that there's no specific loading order requirement between Circuitbox and Moneta.

While it's correct there's no specific loading order requirement when using Moneta with Circuitbox, Circuitbox won't take care of loading Moneta if someone wants to use it. Perhaps there's a different way to word this which reduces confusion but I think something about this is important to mention as Circuitbox 1.x would load (require) Moneta and also had Moneta as a direct dependency in the gemspec.

The other changes to the Readme look good to me and do seem to simplify the Moneta section a bit

@neilvcarvalho
Copy link
Contributor Author

Thanks! That gives some context I didn't have. I could see that Moneta was a direct dependency at some point and that the commit that changed it was in 2019 (so, long ago), but didn't notice that it was released 2 years ago (so, not that long ago).

I can see two options here, using my own judgement. They aren't mutually exclusive:

  1. Append "To use Moneta, add it to your project dependencies." after "for alternative storage options"
  2. Highlight that Moneta is not a direct dependency anymore in the upgrade guide. I couldn't find it there.

I think both are important, as the first satisfies new users (me included), and existing users that are in the upgrade process.

I'm going to change the README right away.

Copy link
Collaborator

@matthewshafer matthewshafer left a comment

Choose a reason for hiding this comment

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

I noticed two cases with inconsistent spacing. I've added suggested changes for them. Otherwise the changes look good to me!

I found the Circuit Store documentation about Moneta integration confusing,
particularly the statement that Moneta "needs to be loaded prior to use."
After testing, I confirmed that there's no specific loading order requirement
between Circuitbox and Moneta.

This commit updates the documentation to:
- Remove potentially confusing language about loading requirements
- Explicitly state that Circuitbox uses an in-memory store by default
- Present Moneta as an optional alternative storage option
- Preserve the existing Moneta compatibility requirements

These changes should help other developers better understand how to integrate
Moneta with Circuitbox if they choose to use an alternative store.
@neilvcarvalho
Copy link
Contributor Author

@matthewshafer Oops, good catch. I pushed an amended commit.

@matthewshafer matthewshafer merged commit 224eecd into yammer:main May 22, 2025
8 checks passed
@matthewshafer
Copy link
Collaborator

@neilvcarvalho Thanks for working on the updates to the readme!

@neilvcarvalho neilvcarvalho deleted the patch-1 branch May 22, 2025 20:36
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.

2 participants

Comments