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

remove default value for hostid parameter #26

Merged
merged 12 commits into from
Oct 24, 2023
Merged

Conversation

lobis
Copy link
Collaborator

@lobis lobis commented Oct 18, 2023

When playing around with fsspec I noticed that when you do not use the required arguments for the given filesystem (e.g. host for protocol="ftp") it throws a TypeError pointing you to the argument you are missing.

>>> import fsspec
>>> fsspec.filesystem(protocol="ftp")
Exception ignored in: <function FTPFileSystem.__del__ at 0x102251d00>
Traceback (most recent call last):
  File "/Users/lobis/miniconda3/envs/uproot-311/lib/python3.11/site-packages/fsspec/implementations/ftp.py", line 248, in __del__
    self.ftp.close()
    ^^^^^^^^
AttributeError: 'FTPFileSystem' object has no attribute 'ftp'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lobis/miniconda3/envs/uproot-311/lib/python3.11/site-packages/fsspec/registry.py", line 289, in filesystem
    return cls(**storage_options)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/lobis/miniconda3/envs/uproot-311/lib/python3.11/site-packages/fsspec/spec.py", line 79, in __call__
    obj = super().__call__(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: FTPFileSystem.__init__() missing 1 required positional argument: 'host'

This PR removes the default value for the hostid parameter. With the default value of "" the instantiation was also giving an error but it was less descriptive and not in line with the other fsspec filesystems.

I also added a test to check the correct type of exception is thrown.

Behaviour after change:

>>> fsspec.filesystem(protocol="root")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lobis/miniconda3/envs/uproot-311/lib/python3.11/site-packages/fsspec/registry.py", line 289, in filesystem
    return cls(**storage_options)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/lobis/miniconda3/envs/uproot-311/lib/python3.11/site-packages/fsspec/spec.py", line 79, in __call__
    obj = super().__call__(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: XRootDFileSystem.__init__() missing 1 required positional argument: 'hostid'

@lobis lobis marked this pull request as ready for review October 18, 2023 22:46
Copy link
Member

@nsmith- nsmith- left a comment

Choose a reason for hiding this comment

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

Thanks!

@lobis
Copy link
Collaborator Author

lobis commented Oct 21, 2023

Looks like the failure comes from before this PR, I tried running the tests locally on the main branch and they appear to fail on the same step (which is weird).

@nsmith- nsmith- merged commit d6f636c into scikit-hep:main Oct 24, 2023
6 checks passed
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.

2 participants