search

rogiersbart / RMODFLOW

Pre- and post-processing of MODFLOW files
https://rogiersbart.github.io/RMODFLOW
32 stars 8 forks source link

Consistent S3 class names #23

Open rogiersbart opened 4 years ago

rogiersbart commented 4 years ago

From the current namespace file, it is obvious we are not really using a consistent approach for naming our S3 classes. We have at least these: bas, bcf, bud, cbc, chd, ddn, de4, dis, drn, evt, ghb, gmg, hed, hfb, hob, hpr, huf, kdep, lmt, lpf, lvda, mlt, modflow, nam, nwt, oc, pcg, pval, rch, riv, rmf_2d_array, rmf_3d_array, rmf_4d_array, rmf_list, rmf_package, sip, upw, wel, zon. I'd like to propose adding the rmf_ prefix everywhere for consistency, and minimizing the potential for conflicts with other packages. Also, I don't think we really have methods in use for rmf_package, but I would think having this there for all objects that can be part of the first level of a full model object (currently modflow I believe, but maybe this should be rmf_model?) might be useful at some point (i.e. we always have the option to use S3 inheritance when it is there). Besides that one, I think it might be useful to introduce an rmf_object one, that is added after e.g. rmf_dis and rmf_package, and serves as parent class for all RMODFLOW S3 objects. This would allow for consistent printing for instance (I'm thinking of using rui::inspect() there, as I would prefer moving the current printing methods to summary methods), by defining a single rmf_object S3 method.

rogiersbart commented 4 years ago

Any thought on this @cneyens ? I don't think it would affect any recent use of the RMODFLOW dev branch, and the S3 classes you are working on in #20 probably all have the prefix already?

cneyens commented 4 years ago

Yes, these all seem like good ideas.

The rmf_package class is only used in nam and the top-level functions for checks but indeed doesn't have any S3 methods atm. Could be used as a subclass of the proposed rmf_object class for printing...

20 converts from/to rmf_list and rmf_2d/3d/4d_array, so this shouldn't affect that branch.