PVL Grammars don't have parameters

[acpaquette]

Describe the bug
The current grammar classes (PVLGrammar, OmniGrammar, ODLGrammar, and ISISGrammar) do not take any of the specified parameters as mentioned in the documentation. Specifically PvlGrammer takes whitespace, reserved_characters, and comments. I believe these parameters would also be usable in the child classes (OmniGrammer, ODLGrammar, and IsisGrammer) however since PvlGrammer can't take advantage of them, neither can the sub classes.

To Reproduce

>>> grammar = pvl.grammar.ISISGrammar(comments=('#'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() got an unexpected keyword argument 'comments'

>>> grammar = pvl.grammar.PVLGrammar(comments=('#'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object() takes no parameters

Expected behavior
When setting these parameters I would expect them to override the defaults being set in the PVLGrammar class any of its subclasses.

Your Environment (please complete the following information):

• OS: macOS 10.13.6
• Environment information: Python 3.6.7
• pvl Version 1.0.0

[rbeyer]

@acpaquette, there are a few things going on here.

1. Yes, the documentation does incorrectly show those items as "parameters" when they are really only attributes of the object. The way that these objects are currently implemented, they do not allow any parameters to be provided when they are instantiated, as your example code tries to do. The easiest fix (although perhaps not the correct fix), is to change the way that they are represented in the documentation.

2. Changing parameters in the current architecture is accomplished by subclassing an existing grammar object, or changing the value of an attribute on an instantiated grammar object. Passing parameters in the argument list at instantiation time would require additional logic to ensure proper data types and values, but is certainly possible.

3. However, this makes me wonder what you're really up to. Your example seems to indicate that you want to use an octothorpe (#) as a comment character, but the OmniGrammar allows for that (although the PVLGrammar doesn't). So are you really just trying to get additional comment characters to be parsed, or are you trying to do something else?

Is it that you've found some files that are created by ISIS which have octothorpe-comments, and the OmniGrammar doesn't parse them, and the ISISGrammar should? If so, then we should amend the ISISGrammar to include them like the OmniGrammar does. There is no "spec" for what kind of PVL-text gets written out by ISIS, and while the ISISGrammar is an attempt to do that, it is definitely a work-in-progress, and is the most likely to need additions.

[acpaquette]

@rbeyer Yes it is from an ISIS product, specifically a CassiniISSNac cube.

  Group = Radiometry
    # Bitweight Correction Parameters
    BitweightCorrectionPerformed = "No: Table converted"
    BitweightFile                = "Not applicable: No bitweight correction"

    # Bias Subtraction Parameters
    BiasSubtractionPerformed     = Yes
    BiasSubtractionMethod        = "Overclock fit"
    NumberOfOverclocks           = 2

    # Dark Current Subtraction Parameters
    DarkSubtractionPerformed     = Yes
    DarkParameterFile            = /usgs/cpkgs/isis3/data/cassini/calibration-
                                   /darkcurrent/nac_median_dark_parameters042-
                                   28.full.cub
    BiasDistortionTable          = /usgs/cpkgs/isis3/data/cassini/calibration-
                                   /darkcurrent/nac_bias_distortion.tab

    # Linearity Correction Parameters
    LinearityCorrectionPerformed = Yes
    LinearityCorrectionTable     = /usgs/cpkgs/isis3/data/cassini/calibration-
                                   /linearize/NAC2.lut

This is more to do with ALE

We are trying to use the ISISGrammar and running into the OmniGrammar comments but also needs the + removed from the reserved_characters that the ISISGrammar provides. I have done what you suggested and updated the comments of the ISISGrammar we are using to include the # in the comments which will work for now.

grammar = pvl.grammar.ISISGrammar()
grammar.comments+=(("#", "\n"), )

Just for further discussion, directly modify an attribute of the class like this is an unusual interface provided by the Grammar classes. Editing the attributes directly like this can lead to exposing pieces of the interface that shouldn't be exposed. I know that there really is no protected interfaces in python but limiting consumers of these classes to a select number of parameters can help to better define what should be editable and what should not be touched. Without the parameters pinning down exactly what attributes of the class I can modify, I end up needing to further examine exactly what the variables are called and what I need to modify.

[rbeyer]

Improving ISISGrammar: when I get the chance, I'll add the ("#", "\n") pair to the ISISGrammar.comments attribute, and fix the documentation (this way you won't have to do the manual add anymore).

You're right that this architecture does not have a user-friendly mechanism for interaction, and that's something that could be improved. The solution will require some work, so may not be quick to accomplish.

[acpaquette]

I think that is a good solution for the time being. It would be nice to get those parameters "properly" exposed but I understand that would take a fair amount of work.

Maybe we can keep this issue around so updating the grammar classes parameters can still be on the table?

[rbeyer]

@acpaquette, agreed. I've created #73 to address the near-term issues that we discussed here, but will keep this issue around to remind us to work on enhancing the grammar interfaces in pvl in the future.

Apologies for being silent, I've had some computer issues for the last month.