param icon indicating copy to clipboard operation
param copied to clipboard

Update objects when path changes for MultiFileSelector and FileSelector

Open maximlt opened this issue 4 years ago • 2 comments

Fixes #545 (only addresses the update path issue, not the default value)

import param

class A(param.Parameterized):
    fs = param.FileSelector(path='/usr/share/*')
    mfs = param.MultiFileSelector(path='/usr/share/*')

a = A()

print(a.fs is None)  # True
paths = a.param.fs.objects.copy()
a.param.fs.path = '/usr/bin/*'
print(a.param.fs.objects == paths)  # False

print(a.mfs is None)  # True
paths = a.param.mfs.objects.copy()
a.param.mfs.path = '/usr/bin/*'
print(a.param.mfs.objects == paths)  # False

maximlt avatar Oct 08 '21 13:10 maximlt

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@090d1c4). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #546   +/-   ##
=========================================
  Coverage          ?   82.13%           
=========================================
  Files             ?        4           
  Lines             ?     2950           
  Branches          ?        0           
=========================================
  Hits              ?     2423           
  Misses            ?      527           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 090d1c4...75dfc79. Read the comment docs.

codecov-commenter avatar Oct 12 '21 17:10 codecov-commenter

@jbednar I've:

  • reverted to the original behavior for default, which is (for both FileSelector and MultiFileSelector) to have default=None if not specified by the user, and that can then be updated by calling '.update()', to 'self.objects[0]' for FileSelector and 'self.objects' for MultiFileSelector
  • Removed _on_set (a method called when an attribute is set on a Parameter) since it was just not working in this case (it was calling .update(), itself using self.path which still pointed to the old attribute value)
  • Updated the user guide to describe these behaviors

maximlt avatar Oct 12 '21 17:10 maximlt

This looks good. @maximlt can you advise if this PR still should be merged and @jbednar can you please either approve and merge or suggest what other fixes are needed?

philippjfr avatar Jan 07 '23 13:01 philippjfr

I think is is ready for revision by @maximlt now that #605 was merged. I think it makes more sense for the default always be the first value or all values (for multi) unless the user has explicitly provided a default of None; None is not a very useful default. In any case, the PR title needs updating to reflect the actual changes in the diffs.

jbednar avatar May 12 '23 21:05 jbednar