Change some of the uses in the TOML library to private #13497
Conversation
This enum is an implementation detail, so now that we can avoid exposing its symbols to users of the module, we should.
TomlParser is not marked "no doc" and has some symbols that are documented and visible, so it does not get the same treatment. This seems to be safe to do based on the contents of the library/packages/TOML/test directory, but I will need a full test run to be certain.
Regexp is used in the implementation of things that are hidden from outside the package module, so does not need to be exposed outside of the context in which it is used. The symbols from the DateTime module are exposed as part of the user-facing interface in the TomlParser module, so should remain publicly used for convenience.
I changed the use of the enum to private in TomlParser, so its contents were no longer visible without prefix. I could have changed it so that mason privately used the enum, but since I only see one such error in each module, that seemed unnecessary.
|
@Spartee - thoughts? |
|
I think that this is good especially since it seems like little work was necessary to change the implementation. Something I foresee being an issue is what @ben-albrecht and I talked about a few days ago where a user needed to read from a toml array into a chapel array. In that case it may be useful to have the enum exposed and documented so that users can take advantage of that functionality. Otherwise I think this is ok for now, but I would like to hear Ben's thoughts. |
|
👍 to this change. We can tackle the use-case @Spartee mentions separately, since it will require bigger changes.
I think supporting this pattern will require some redesign of the TOML interface, hopefully an extension of the existing interface rather than any backwards-breaking changes. Currently, the TOML module has been designed for reading TOML files with arbitrary structure, which is the core pattern used in Mason. As a result, all fields end up being read or written as a string eventually. A different pattern is when the user knows the TOML structure at compile-time, and wants to read in the values as the types themselves, such as an array of ints. For this use-case, we may want to give users a way to specify the expected types to be read in, similar to the CSV interface designed by @daviditen. @npadmana recently encountered this shortcoming in the TOML interface. I'll make a note to open a separate issue on this if it doesn't exist already. |
This changes the use of TomlReader, Regexp, and fieldtag to private.
TomlReader is a "no doc"ed submodule, Regexp is only used in the
implementation of the functions that use it, and fieldtag is a
"no doc"ed enum.
It leaves the uses of TomlParser and DateTime public. The former is
a submodule with documented portions, while DateTime's types are
used as the type of arguments for some of its public functions.
As a result of making the use of the "no doc"ed enum private, the
Mason support files that referenced the enum constant needed to
either use the enum themselves, or add the enum name prefix. Since
only one constant was used in each of those files once, I chose to
just add the enum name prefix. It might be worth considering whether
this enum should be public and documented due to its use in other
places than just the TOML module . . .
Passed a full paratest with futures