Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow models to have mixed case filenames in help #137

Merged
merged 1 commit into from Jan 16, 2018

Conversation

llimeht
Copy link
Contributor

@llimeht llimeht commented Jan 9, 2018

sasmodels contains filenames that are not all lowercase (unified_power_Rg.py) and so the HTML help pages generated for this model are also mixed case (unified_power_Rg.html). The current code forces the filename for the HTML help file to lowercase means that the help for this model will not be found.

2018-01-09 09:38:41,256 : INFO : root (kernelpy.py:36) :: load python model unified_power_Rg
2018-01-09 09:38:44,161 : ERROR : sas.sasgui.guiframe.documentation_window (documentation_window.py:102) :: Could not find Sphinx documentation at /usr/lib/python2.7/dist-packages/sas/sasview/doc/user/models/unified_power_rg.html -- has it been built?
$ ls -l /usr/lib/python2.7/dist-packages/sas/sasview/doc/user/models/
...
-rw-r--r-- 1 root root  9639 Jan  7 12:46 unified_power_Rg.html
...

This can either be fixed by renaming the file in sasmodels (and requiring that all models are always lowercase filenames which would need to be enforced in the test suite), or by dropping the conversion to lowercase. I can't see any reason to want to convert to lowercase and the commit that added that code does not say why it was considered necessary. Dropping the call to lower() seems like the right approach.

http://bugs.debian.org/886715

sasmodels contains filenames that are not all lowercase (unified_power_Rg.py)
and so the HTML help pages generated for this model are also mixed case
(unified_power_Rg.html). Forcing the filename to lowercase means that the
help for this model will not be found.

http://bugs.debian.org/886715
@pkienzle
Copy link
Contributor

pkienzle commented Jan 9, 2018

Another option is to force the generated html file to use lower case so that the help button can find the file. Anybody have an opinion on which is better?

@llimeht
Copy link
Contributor Author

llimeht commented Jan 10, 2018

The options appear to be:

  • Alter the .rst filenames where they are calculated in sasmodels/doc/Makefile so it is only done once. Converting to lowercase in make is difficult to do without going via the shell to use tr, sed or similar, and I assume that's not an acceptable loss of portability.
  • Add case squashing in both sasmodels/doc/genmodel.py and sasmodels/doc/gentoc.py (and also other places we've potentially forgotten or get added later). Trying to exhaustively find all places it is required seems like a source of future bugs.
  • Eliminate the need for changing case (renaming unified_power_Rg to be lowercase) and add in a check to sasmodels/models/core.py:list_models() that insists on model names being lowercase so that this doesn't reappear with some future model.
  • Remove all places where case conversion is performed. (this PR)

@butlerpd
Copy link
Member

As noted on the fortnightly developer's call, the decision was made when converting to the new model infrastructure that nomenclature standards should be introduced and enforced. The decision regarding model names was in fact that it NOT include camel case and be all lower case as noted in the documentation in several places. In particular the model conversion instructions at:
http://trac.sasview.org/wiki/ModelConvInst

Indicate that

* No capitalization and thus no CamelCase. If necessary use underscore to separate (i.e. barbell not BarBell or broad_peak not BroadPeak)
* Remove “model” from the name (i.e. barbell not BarBellModel)

Thus whoever converted the Beaucage model failed to adhere to the standard and the consensus was that the immediate action should be to change that to rg and a ticket has been added to trac (ticket 1059) to make sure it is not forgotten.

That said, it is clear that at some point a contributor to the marketplace may not notice that wee little rule and create a model that does not adhere to the standard. Question then is how it should be handled (assuming the plugins suffer from the same issue - though currently I don't think plugin docs are actually created? so no help file available?) There probably needs to be some further, broader discussion on what solution should be implemented in SasView.

@llimeht
Copy link
Contributor Author

llimeht commented Jan 10, 2018

If matching the developer documentation implies renaming unified_power_Rg.py, does that break saved project files? (A quick test makes me think it does. Something tells me that's not OK either.)

@pkienzle
Copy link
Contributor

The relevant commit was for a mac doc issue in SasView 3.1:

2015-06-23 2c8dc1998 [paul butler] convert model name to lower case - this should fix mac issue.  Also add space between name and "help" to go in panel title

https://github.com/llimeht/sasview/commit/2c8dc199897f6f98c46091ea563218c898baf57a

Enough has changed with the move to sasmodels that lower casing the name may no longer be necessary, and this PR is the correct solution.

Or we could force all names to lowercase internally, including when reading a saved model. Most Windows & Mac users have a case insensitive file system, so effectively it treats upper and lower case names the same already. Since the list of available models is built up by scanning a couple of directories, this should not change the functionality.

My inclination is to apply this PR and fix whatever breaks. Either way, I suspect there will be more bugs related to this issue.

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to check the docs for unified_power_Rg on both windows and mac after this change has been applied. This includes the links within the html docs as well as the help button. Check the PDF as well in case it has internal links.

@llimeht
Copy link
Contributor Author

llimeht commented Jan 12, 2018

Either way, I suspect there will be more bugs related to this issue.

Agreed, these tend to be the sorts of gifts that keep on giving.

I wonder if some sort of help_file_path() function would be worthwhile (perhaps within sasmodels?) such that the accuracy of the help filename calculation can be tested and perhaps even the ability to generate the documentation from the source code can become a testable thing too.

Be sure to check the docs for unified_power_Rg on both windows and mac after this change has been applied. This includes the links within the html docs as well as the help button. Check the PDF as well in case it has internal links.

I will end up checking through the docs on the Linux side with a case-sensitive filesystem; I can't do other operating systems. I suspect that the combination of gentoc.py and sphinx will make sure that everything is internally self-consistent within those documents.

@butlerpd
Copy link
Member

@llimeht asks

If matching the developer documentation implies renaming unified_power_Rg.py, does that break saved project files? (A quick test makes me think it does. Something tells me that's not OK either.)

Sadly that is already a solved problem in that during the conversion to 4.0 we had to put out a release before all name changes were complete. Thus we already have the infrastructure to deal with this backward compatibility and have a number of such model name changes. This then becomes a separate issue from whether to support names that do not adhere to the standard. Probably should if we can.

@pkienzle
Copy link
Contributor

Given that the unified_power_Rg ticket has been added, this PR no longer depends on it, so merging.

@pkienzle pkienzle merged commit e110cb0 into SasView:master Jan 16, 2018
@llimeht llimeht deleted the tmp/htmllowercase branch January 18, 2018 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants