Stop save_locally defaulting to the installed package directory - #185
Open
caglarpir wants to merge 1 commit into
Open
Stop save_locally defaulting to the installed package directory#185caglarpir wants to merge 1 commit into
caglarpir wants to merge 1 commit into
Conversation
save_locally declared:
file_path: str = os.path.dirname(os.path.realpath(__file__))
That default is evaluated once at import, so it resolves to the
directory containing interface.py -- i.e. inside site-packages. Calling
save_locally() without file_path therefore wrote user data into the
installed package rather than anywhere the caller would expect. The
docstring already claimed it defaults to the current directory, so the
documented behaviour and the actual behaviour disagreed.
Reproduced before the fix, from an unrelated working directory:
written to: .../site-packages/mapillary/probe_write.geojson
cwd: []
Beyond being surprising, writing there can fail on a system-wide
install, silently pollutes the installed package, and loses the files on
reinstall or uninstall.
Defaults file_path to None and resolves it to os.getcwd() at call time,
which is what the docstring already described. Explicit file_path values
are unaffected. Docstring examples that passed the package directory
have been changed to a plain relative path so they no longer suggest the
old behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
save_locallydeclared:Python evaluates default arguments once, at import. So this resolves to the directory containing
interface.py— inside site-packages. Callingsave_locally()without an explicitfile_pathwrote user data into the installed package.The docstring already said "Defaults to the current directory path", so the documented and actual behaviour disagreed.
Reproduction
From an unrelated working directory, on current
main:The file lands in site-packages; the working directory stays empty.
Beyond being surprising, writing there can fail outright on a system-wide install where site-packages is not user-writable, silently pollutes the installed package, and loses the saved files on reinstall or uninstall.
Fix
Default
file_pathtoNoneand resolve it toos.getcwd()at call time — which is what the docstring already described. Explicitfile_pathvalues are unaffected.The two docstring examples that passed
os.path.dirname(os.path.realpath(__file__))now use a plain relative path, so they no longer suggest the old behaviour.Behaviour change
This is a deliberate behaviour change, so it is split out from the metadata fixes in #184 rather than bundled. Anyone who was relying on the old default was relying on files landing in site-packages, which seems very unlikely to be intentional — but it is a change, and worth a maintainer’s eye rather than being slipped in.
Verified after the fix
Unrelated bug noticed nearby (not fixed here)
Saving an empty
FeatureCollectionas CSV printsAn error occurred: list index out of rangeand writes nothing. I confirmed this reproduces on unpatchedmain, so it predates this change — the CSV controller does not handle zero features and swallows the exception with aprint. Left alone to keep this PR to one concern; happy to file it separately.Test plan
pytest— 17 passedblack --checkclean on the modified file,flake8 --select=E9,F63,F7,F82— 0save_locallysignature change