inpowell / modulartabler

MIT License
3 stars 0 forks source link

Shrink MappingTable interface #4

Open inpowell opened 2 months ago

inpowell commented 2 months ago

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 commented 2 months ago

It looks like the interface I have settled on is:

MappingTable

inpowell commented 2 months ago

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 commented 2 months ago

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.