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

Shrink MappingTable interface #4

Open
10 tasks
inpowell opened this issue Aug 14, 2024 · 3 comments
Open
10 tasks

Shrink MappingTable interface #4

inpowell opened this issue Aug 14, 2024 · 3 comments

Comments

@inpowell
Copy link
Owner

inpowell commented Aug 14, 2024

The interface for mapping tables exposes a lot of implementation details, and I don't want to support them publicly.

Public

  1. initialize() definitely needs to be public
  2. print() must be public
  3. count_aggregate() must be public, because that's the functionality we want!
  4. preprocess() is likely only to be used by count_aggregate(), so I am leaning towards making this private.

Active bindings

  1. map is only important for count_aggregate(). It might need to be extended when implementing dbplyr backends
  2. mtab should be available to create an output table skeleton (and allow sorting so nullspace is in the right order).
  3. The only use of matrix is to get nullspace
  4. Information in table_cols will already be available from mtab('s replacement)
  5. join_clause is an implementation detail (and I want to change it back to a character type for dtplyr compatibility), so can be made private
  6. nullspace is a major part of what we need for cell suppression (the only other part being what we want to suppress)

Interestingly, private superclass methods are available to subclasses as private$method() or super$method(), but private fields are not.

@inpowell
Copy link
Owner Author

inpowell commented Aug 22, 2024

It looks like the interface I have settled on is:

MappingTable

  • Public methods

    • initialize(...)
    • print(...)
    • preprocess(data, ...)
    • count_aggregate(data, wt = NULL, ..., name = 'n')
    • get_result_skeleton(...)
    • get_nullspace(...)
    • nullspace (active binding, defaulting to self$get_nullspace() with no arguments.
  • Protected/private methods

    • get_input_cols()
    • get_output_cols()
    • get_mapping_table(...)
    • get_by()
    • get_matrix()

@inpowell
Copy link
Owner Author

The current implementation for MultiMappingTable needs basically everything from BaseMappingTable to be public. The necessary feature would be "friend"-type access, which R6 doesn't support (and likely never will). I thought we'd be able to do something with super, but that only works for the self-analogue.

If we really wanted a reduced interface, I can't see a good way to do this other than S4, and I think that's probably not worth it.

@PeterM74
Copy link
Collaborator

Ah that's a shame. I don't think it is a big issue as there are plenty of packages building objects in S3 where everything is just kept in a big list wrapped in an S3 class and the user just ignores the elements they don't need 😄 An alternative could be to create the 'public' options as intended, and create one last public called .Internals or something that contains everything that the user probably won't need? At least this compacts the resulting object for the user into things they should care about and things they shouldn't.

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

No branches or pull requests

2 participants