Plaintext Reader Docs are wrong

[JustinPrivitera]

The code example here is wrong:

plainTextOpenOptions = GetDefaultOpenOptions()

should be

plainTextOpenOptions = GetDefaultFileOpenOptions()

[DecodersLord]

Hello, I see that the code is being generated from "visit/src/test/tests/databases
/plaintext.py" do you want that code to be tested first initially or it's just straight document change and only need the file to be updated to use GetDefaultFileOpenOptions()?

Do I need the issue to be assigned before I start working and put in a PR?

[JustinPrivitera]

Hi @DecodersLord thanks for jumping in here!

Do I need the issue to be assigned before I start working and put in a PR?

Nope, not at all! It looks like you went ahead and got started so that's great.

Hello, I see that the code is being generated from "visit/src/test/tests/databases
/plaintext.py" do you want that code to be tested first initially or it's just straight document change and only need the file to be updated to use GetDefaultFileOpenOptions()?

This is unfortunate. I didn't look into this when I opened the ticket. I assumed it was merely a code example in the docs that was incorrect, and I didn't think that it was borrowing code from our tests. It looks like we don't want to change our tests for reasons listed in the test file: https://github.com/visit-dav/visit/blob/develop/src/test/tests/databases/plaintext.py#L38

I'm at a loss for how to advise here. I will discuss with the other @visit-dav/visit-developers and get back to you.

[DecodersLord]

Thank you for your response @JustinPrivitera... I will wait for your response and can make other changes if required.

[markcmiller86]

@JustinPrivitera if the problem is that the symbol named used in the test and ultimately then appearing in the documentation is misleadingly similar to but ultimately different from the actualy CLI function a user should call, I think there are some simple remedies.

• Add a comment in the code kinda sorta like so...

   # GetDefaultOpenOptions is a wrapper to the CLI method GetDefaultFileOpenOptions
   # which ensures the options are deep copied before they are set
   plainTextOpenOptions = GetDefaultOpenOptions()
   plainTextOpenOptions['First row has variable names'] = 1
   plainTextOpenOptions['Data layout'] = '2D Array'
   SetDefaultFileOpenOptions("PlainText", plainTextOpenOptions)
  
  

• Adjust the python code a bit and essentially rename GetDefaultOpenOptions() to GetDefaultFileOpenOptions() so that when it is included in the docs, a reader will be none the wiser that it is really overriding (solely for convenience of testing). Ok, how do you do that because the CLI already defines GetDefaultFileOpenOptions()...

  visitCLIOriginalMethod = GetDefaultFileOptionOptions
  def GetDefaultFileOpenOptions():
      if not hasattr(GetDefaultFileOpenOptions, 'defaultOpenOptions'):
          GetDefaultFileOpenOptions.defaultOpenOptions = copy.deepcopy(visitCLOriginalMethod("PlainText"))
      return copy.deepcopy(GetDefaultFileOpenOptions.defaultOpenOptions)
  

[JustinPrivitera]

Thanks for chiming in @markcmiller86! Those are great solutions.

[markcmiller86]

Thanks for chiming in @markcmiller86! Those are great solutions.

Sure. That code is really trying to serve a few different masters...to be a faithful representation of the actual CLI code and to be easily included in our documentation and to be testable and tested with our nighly tests.

All that can create situations where we present to users in our docs code that doesn't quite faithfully represent the CLI and when we discover such situations, we should correct them as you have identified here.

[DecodersLord]

Thank you for the solutions @markcmiller86.
@JustinPrivitera Is there any preference for the solution? I think adding a comment on the code section is easy and protects the test code against any issues down the road.

I guess you can decline my previous PR and I will put in a new one?

[JustinPrivitera]

@JustinPrivitera Is there any preference for the solution?

@DecodersLord I think either would be fine. The first is nice and simple. I would ensure the comment explains that GetDefaultOpenOptions() is a function internal to our tests.

I guess you can decline my previous PR and I will put in a new one?

No need for that; you can just make changes on the branch that you already have.