Raise a TypeError if wrong type given to fix_url

[eode]

Description

This is in relation to "Package.set_dir() should be more restrictive to path".

Note that I've modified fix_url instead of set_dir(). fix_url() is the centralized place for checking local-style paths. All tests passed with this small change, but it could cause people to need to adapt, if they have code dependent on the previous behavior.

If changing this here is considered too broad, I can shift it into set_dir.

TODO

• Unit tests
• [n/a] Automated tests (e.g. Preflight)
• Documentation
  • Python: Run build.py for new docstrings
  • [n/a] JavaScript: basic explanation and screenshot of new features
• [?] Changelog entry

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.36%. Comparing base (1823aa6) to head (9cf11b9).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2936      +/-   ##
==========================================
+ Coverage   38.35%   38.36%   +0.01%     
==========================================
  Files         778      778              
  Lines       34366    34374       +8     
  Branches     5219     5424     +205     
==========================================
+ Hits        13180    13188       +8     
+ Misses      20643    20007     -636     
- Partials      543     1179     +636     

Flag	Coverage Δ
api-python	91.35% <100.00%> (+<0.01%)	⬆️
catalog	16.87% <ø> (ø)
lambda	91.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eode]

It could be worth looking up functions that use fix_url and updating their docstrings as well. Or, moving the change out of fix_url and into set_dir(). But, this seems like an opportunity to start migrating to universal behavior for raising errors for places where we accept an FS path.

[sir-sigurd]

I'm not yet sure whether we should touch fix_url(), but there are some thoughts:

• supporting PathLike is a good idea
• do we have some tests with PathLike?
• When we know that a value is a path, but not a URL, I think it's preferred to use PhysicalKey.from_path() rather than PhysicalKey.from_url() like here:
  

  quilt/api/python/quilt3/packages.py

  Lines 844 to 847 in 5f2ec40

  	if path:
  	src = PhysicalKey.from_url(fix_url(path))
  	else:
  	src = PhysicalKey.from_path(lkey)

  
  So it might make sense to check whether path is instance of PathLike and use PhysicalKey.from_path() in that case.

[eode]

@sir-sigurd Ran across something.

(master) This is a weird behavior:

>>> p = Package()
>>> p.set_dir('example')
>>> p.set_dir('')
# Result: Package is set to PWD.

>>> p = Package()
>>> p.set_dir('')
>>> p.set_dir('example')
# Result: Package is set to PWD.

That seems like buggy behavior. Is it actually intentional?

Extended example:

>>> p = Package()
>>> p.set_dir('')
(local Package)
 └─bar
 └─example/
   └─baz
   └─bing
 └─foo
>>> p.set_dir('example')
(local Package)
 └─bar
 └─example/
   └─baz
   └─bing
 └─foo
>>> p = Package()
>>> p.set_dir('example')
(local Package)
 └─example/
   └─baz
   └─bing
>>> p.set_dir('')
(local Package)
 └─bar
 └─example/
   └─baz
   └─bing
 └─foo

I'd imagine that it should
a: always set the package dir, or
b: set the package dir once and create an error in other circumstances.

Can you specify what the behavior should be? I'm working on that chunk of code, and may as well fix this if it's a bug.

[sir-sigurd]

I don't understand what's unexpected. From docs:

Package.set_dir(self, lkey, path=None, meta=None, update_policy='incoming')
...
path(string): path to scan for files to add to package. If None, lkey will be substituted in as the path.

Does this make sense?

[eode]

Nevermind, I was thinking of set_dir as setting the package contents to the specified tree, rather than adding the specified tree to the package. Still an RTFM issue, though -- my mistake.