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

Raise an error if function name is blank for New Plugin Model (Trac #770) #884

Closed
pkienzle opened this issue Mar 30, 2019 · 10 comments
Closed
Assignees
Labels
Enhancement Feature requests and/or general improvements Minor Small job
Milestone

Comments

@pkienzle
Copy link
Contributor

pkienzle commented Mar 30, 2019

There is no error if the new plugin model name is blank, but the resulting function won't show up in the list of custom models.

Default the name to blank so that the user is required to provide a meaningful name.

Migrated from http://trac.sasview.org/ticket/770

{
    "status": "closed",
    "changetime": "2016-11-08T10:24:22",
    "_ts": "2016-11-08 10:24:22.915372+00:00",
    "description": "There is no error if the new plugin model name is blank, but the resulting function won't show up in the list of custom models.\n\nDefault the name to blank so that the user is required to provide a meaningful name.",
    "reporter": "pkienzle",
    "cc": "",
    "resolution": "fixed",
    "workpackage": "SasView Bug Fixing",
    "time": "2016-10-10T23:56:44",
    "component": "SasView",
    "summary": "Raise an error if function name is blank for New Plugin Model",
    "priority": "minor",
    "keywords": "",
    "milestone": "SasView 4.1.0",
    "owner": "piotr",
    "type": "enhancement"
}
@pkienzle pkienzle added this to the SasView 4.1.0 milestone Mar 30, 2019
@pkienzle pkienzle added Enhancement Feature requests and/or general improvements Incomplete Migration Minor Small job and removed Incomplete Migration labels Mar 30, 2019
@smk78
Copy link
Contributor

smk78 commented Mar 30, 2019

Trac update at 2016/10/11 17:12:43: smk78 commented:

The same issue applies to the Sum|Multi case too.

@smk78
Copy link
Contributor

smk78 commented Mar 30, 2019

Trac update at 2016/10/11 17:13:30:

  • smk78 changed _comment0 from "Don't understand how starting out with a blank function name helps any though?" to "1476206384171087"
  • smk78 commented:

Don't understand how starting out with a blank function name helps any though? Wouldn't it be better to test for an empty string?

@butlerpd
Copy link
Member

Trac update at 2016/10/11 17:18:28: butler changed type from "defect" to "enhancement"

@rozyczko
Copy link
Member

Trac update at 2016/10/12 01:20:21: piotr commented:

Indeed it would be better to start with a predefined (default) name - the less the user has to add/modify the more "friendly" the control is, I guess.

@rozyczko
Copy link
Member

Trac update at 2016/10/12 01:20:44:

  • piotr changed owner from "" to "piotr"
  • piotr changed status from "new" to "accepted"

@rozyczko
Copy link
Member

Trac update at 2016/10/12 01:22:54: piotr commented:

Do we need to worry about the backend? Will sasmodels catch attempts to pass ".py"?

@sasview-bot
Copy link

Trac update at 2016/10/12 01:24:00: Piotr Rozyczko <[email protected] commented:

In changeset ad1ac45:

#!CommitTicketReference repository="sasview" revision="ad1ac45c1b0fae0a90dcd973e0dcaa6368726c9a"
filter out empty filenames. Fixes the obvious part of #884

@pkienzle
Copy link
Contributor Author

Trac update at 2016/10/13 05:37:34: pkienzle commented:

Replying to [comment:4 piotr]:

Indeed it would be better to start with a predefined (default) name - the less the user has to add/modify the more "friendly" the control is, I guess.

Reasons to leave the initial name blank:

  • There is no meaningful default name.

  • The current default of "!MyFunction" works the first time, but the next time it will trigger an error because "override" is false (but see below^*^).

  • Users who have created a model of the name "!MyFunction" and want to share it on the marketplace will have to contend with other users who just left the default name.

Even if we do stay with a default name, the name "!MyFunction" does not match the underscore naming style of the new models.

^*^Override is currently True, which is bad since it can destroy the previously created model. The default was changed to from False in response to the error on "Apply" when the model was incorrect; it should be change back to False to avoid accidental deletion of models.

@smk78
Copy link
Contributor

smk78 commented Mar 30, 2019

Trac update at 2016/10/13 11:10:08: smk78 commented:

I tend to side with PaulK that NOT providing a name is best. But I think the issue here is that we don't provide any way from this window (or the Sum|Multi window) for the user to see what names are already in use - think how a normal load/save dialog box works. If we could let them see what was already there then PaulK's concern about my changing the default action of overwrite is also mitigated. But I found it, during testing, profoundly annoying that every little coding slip up required me to give a new model name until I understood what the overwrite box did. In which case, if you're providing an overwrite capability I don't see why you wouldn't enable it...

@sasview-bot
Copy link

Trac update at 2016/11/08 10:24:22:

In changeset 94f0873:

#!CommitTicketReference repository="sasview" revision="94f0873f0d6db34d039295cbc4fbe5a7bf2b1467"
Removed default name and changed override status. Fixes #884

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Feature requests and/or general improvements Minor Small job
Projects
None yet
Development

No branches or pull requests

5 participants