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

Make ModuleScope internal #557

Closed
wants to merge 6 commits into from
Closed

Conversation

stakx
Copy link
Member

@stakx stakx commented Jan 6, 2021

Following up from our discussion in #389 (comment), I'm proposing to make ModuleScope internal. This affects mainly SaveAssembly and LoadAssemblyIntoCache, as everything else is already internal. Those methods need new public entrypoints.

Loading a previously generated assembly into the type cache

This would now be done via IProxyBuilderWithCache.LoadAssemblyIntoCache(Assembly), which is implemented in DefaultProxyBuilder.

Introducing a new interface IProxyBuilderWithCache feels like a clean solution as long as IProxyBuilder is a public abstraction... there could be user implementations out there (unlikely, I know) that don't have a type cache. But it might also be overkill. Perhaps it's OK if such implementations simply throw NotSupportedException for the new load method; then the method could sit directly in IProxyBuilder.

Saving a generated assembly

This can already be done through PersistentProxyBuilder (on platforms supporting AssemblyBuilder.Save).

However, there would now be no way to specify the desired target file path, nor the assembly's internal name. We might want to expose those capabilities through different overloads, e.g.

-string SaveAssembly();
+void SaveAssembly(string path = ModuleScope.DEFAULT_FILE_NAME);
+void SaveAssembly(string name, string path = ModuleScope.DEFAULT_FILE_NAME);

Closes #389. There's still a few bits we could remove from the public API (in some cases we already have separate issues), but by and large, we're done.

P.S.: This assumes IProxyBuilder stays public. It if shouldn't, everything here would have to move one level up (into IProxyGenerator, where it admittedly doesn't fit quite as nicely from a conceptual POV).

@stakx stakx requested a review from jonorossi January 6, 2021 14:33
@stakx stakx force-pushed the module-scope branch 6 times, most recently from dc6f026 to dbc79c0 Compare January 6, 2021 15:02
@stakx stakx added this to the v5.0.0 milestone Jan 6, 2021
Comment on lines +24 to +26
public interface IProxyBuilderWithCache : IProxyBuilder
{
#if FEATURE_SERIALIZATION
Copy link
Member Author

Choose a reason for hiding this comment

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

Due to the ordering of this #if, targets that don't support full binary serialization still get this interface (but it will be empty). I chose this—rather than erasing the interface as a whole—because it is true on all targets to say that "DefaultProxyBuilder is a builder with a cache", even when you cannot batch-populate it. It also gives us more API uniformity, as DefaultProxyBuilder will implement the same interfaces for all targets.

stakx added 6 commits January 19, 2021 21:29
This has the side benefit of better validation of deserialized data, as
generation now has to pass through the existing proxy type validation
logic in `DefaultProxyBuilder`.
`LoadAssembliesIntoCache` only makes sense for proxy builders that have
a type cache. Since the latter isn't exposed in `IProxyBuilder`, we are
adding it to a new specialized sub-contract, `IProxyBuilderWithCache`.
@stakx stakx closed this Apr 21, 2021
@stakx stakx removed this from the v5.0.0 milestone Apr 27, 2021
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.

Reduce public API surface
1 participant