param icon indicating copy to clipboard operation
param copied to clipboard

class Foldernames and user guide snippet

Open jmborr opened this issue 2 years ago • 13 comments

Signed-off-by: Jose Borreguero [email protected]

jmborr avatar Dec 16 '22 23:12 jmborr

@jbednar @philippjfr @jlstevens Hi, would you consider adding this Foldernames class to param ?

jmborr avatar Dec 17 '22 02:12 jmborr

Thanks! It does seem reasonable to support multiple folder names. Is there a good reason to allow or support a single string rather than always requiring a list and returning a list for __get__? Seems much simpler for downstream code and easier to describe to users.

jbednar avatar Dec 20 '22 21:12 jbednar

Thanks! It does seem reasonable to support multiple folder names. Is there a good reason to allow or support a single string rather than always requiring a list and returning a list for __get__? Seems much simpler for downstream code and easier to describe to users.

We had a request from a User for a parameter allowing one to pass either one or multiple directories to it. In the case of one directory, it's true I could always pass a one-item list, i.e, Foldernames([mydir]) but this does look unnecessarily complex and unintuitive, hence the option to also allow Foldernames(mydir).

jmborr avatar Dec 20 '22 23:12 jmborr

The problem is that a user of this parameter would then always have to check if it was a list or not, when actually accessing the value. The way around that would be to promote a single item provided into a one-item list before storing it or at least before accessing it, which seems ok.

jbednar avatar Dec 21 '22 05:12 jbednar

The problem is that a user of this parameter would then always have to check if it was a list or not, when actually accessing the value. The way around that would be to promote a single item provided into a one-item list before storing it or at least before accessing it, which seems ok.

If I understand you, the interface would be:

  • able to set the parameter either with a list or with a string
  • get the parameter as a list, always

I think this interface is better because, as you say, you know in advance the type you'll get. If that sounds OK by you, then I'll make the changes (and hopefully fix the tests 🙂 )

jmborr avatar Dec 21 '22 10:12 jmborr

Sounds good, thanks!

jbednar avatar Dec 21 '22 12:12 jbednar

@jbednar can you please take a second look? By the way, I'm getting a linting error when linting with python 2.7 at this line:

def _resolve(self, paths: FlexPaths):

I assume it's because of the type hint. Should I remove all type hints so that the code can support the deprecated python 2.7 :scream_cat: ?

jmborr avatar Dec 31 '22 10:12 jmborr

And yes, we haven't yet dropped Python 2.7 support in Param, so please remove py3 syntax until we release Param 2.0.

jbednar avatar Jan 04 '23 22:01 jbednar

And yes, we haven't yet dropped Python 2.7 support in Param, so please remove py3 syntax until we release Param 2.0.

I removed the Python type hints

jmborr avatar Jan 08 '23 15:01 jmborr

Looks good! almost ready to merge. If you want this merged in a 1.X release, we'd have to make importing pathlib conditional. Otherwise, for param 2.0

I think better leave it for param 2.0. In this case, should the PR be against other branch than main?

jmborr avatar Jan 09 '23 17:01 jmborr

No, I think the plan is for us to release a last patch or minor release for 1.x pretty soon and then create a branch-1.x which will be used for urgent backports and fixes. From then on main will serve as the development branch for 2.0.

philippjfr avatar Jan 09 '23 17:01 philippjfr

No, I think the plan is for us to release a last patch or minor release for 1.x pretty soon and then create a branch-1.x which will be used for urgent backports and fixes. From then on main will serve as the development branch for 2.0.

It's OK by me if this is implemented in param 2.0

jmborr avatar Jan 09 '23 18:01 jmborr

I see this is allowing pathlib.Path paths, which is great, but I'd like Param to accept that consistently where it makes sense and not just in some Parameters. Also, if we add Foldernames, I guess we should add Filenames too :) Choosing then not to add that to 2.0 but hopefully shortly after that in 2.1.

maximlt avatar Apr 05 '23 10:04 maximlt