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

Add more classes to the Public API #717

Merged
merged 6 commits into from
Oct 22, 2023
Merged

Add more classes to the Public API #717

merged 6 commits into from
Oct 22, 2023

Conversation

BobLd
Copy link
Collaborator

@BobLd BobLd commented Oct 22, 2023

As discussed in #656.

I've done the changes in several commits - that might help the review. I'll squash them when merging.

I've also update NamedDestinations namespace to match folder structure. Feel free to let me know if that should not the case.

For the moment, IPageFactory stays internal, mainly because InternalParsingOptions is. Same for IResourceStore (and related XObjectFactory).

@EliotJones what's your view on InternalParsingOptions? I don't want to make it public but we will need to find a way to make IResourceStore and IResourceStore public

@BobLd BobLd requested a review from EliotJones October 22, 2023 15:40
{
return null;
}

/// <summary>
/// The page media box.
/// </summary>
public MediaBox MediaBox { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Could these property setters be internal to stop consumers messing with them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well spotted, now updated

@EliotJones
Copy link
Member

@BobLd on IResourceStore an parsing options afaict from a quick scan it should be fine to move them to the constructor of the resource store instance since we have the options at that stage

@BobLd
Copy link
Collaborator Author

BobLd commented Oct 22, 2023

@EliotJones good point on the resource store instance, I'll look into it and create another PR for that. Let's keep this one simple

Copy link
Member

@EliotJones EliotJones left a comment

Choose a reason for hiding this comment

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

LGTM

@BobLd BobLd merged commit 7ab3a6a into master Oct 22, 2023
1 check passed
@BobLd BobLd deleted the render-refactor-1 branch October 22, 2023 16:34
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