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

class Foldernames and user guide snippet #669

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jmborr
Copy link

@jmborr jmborr commented Dec 16, 2022

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

@jmborr
Copy link
Author

jmborr commented Dec 17, 2022

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

@jbednar
Copy link
Member

jbednar commented Dec 20, 2022

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.

@jmborr
Copy link
Author

jmborr commented Dec 20, 2022

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).

@jbednar
Copy link
Member

jbednar commented Dec 21, 2022

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.

@jmborr
Copy link
Author

jmborr commented Dec 21, 2022

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 🙂 )

@jbednar
Copy link
Member

jbednar commented Dec 21, 2022

Sounds good, thanks!

Signed-off-by: Jose Borreguero <[email protected]>
@jmborr
Copy link
Author

jmborr commented Dec 31, 2022

@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 🙀 ?

param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
@jbednar
Copy link
Member

jbednar commented Jan 4, 2023

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

@jmborr
Copy link
Author

jmborr commented Jan 8, 2023

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

@philippjfr philippjfr added this to the 2.0 milestone Jan 9, 2023
Copy link
Member

@droumis droumis left a comment

Choose a reason for hiding this comment

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

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

param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
Signed-off-by: Jose Borreguero <[email protected]>
@jmborr
Copy link
Author

jmborr commented Jan 9, 2023

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?

@philippjfr
Copy link
Member

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.

@jmborr
Copy link
Author

jmborr commented Jan 9, 2023

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

@maximlt
Copy link
Member

maximlt commented Apr 5, 2023

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 maximlt modified the milestones: 2.0, v2.x Apr 5, 2023
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.

5 participants