[generator] download_file() now allows URLs with "file" protocol #3382

Merged

mrater merged 6 commits from test6 into master

2022-09-13 21:30:54 +00:00

mrater commented

2022-09-09 16:09:57 +00:00

(Migrated from github.com)

download_file(url) function now allows URLs beginning with file:// and pointing to a local path.

As the maps_generator README states, In each field where you need to specify a URL, you can specify the path to the file system using file:///path/to/file. It was partially not properly implemented, as download_file was called with URLs using file:// and it tried to get them with GET request.

I've also added the aforementioned note to the maps_generator default ini file.

download_file(url) function now allows URLs beginning with `file://` and pointing to a local path. As the maps_generator README states, _`In each field where you need to specify a URL, you can specify the path to the file system using file:///path/to/file`_. It was partially not properly implemented, as download_file was called with URLs using `file://` and it tried to get them with GET request. I've also added the aforementioned note to the maps_generator default ini file.

biodranik (Migrated from github.com) requested changes 2022-09-10 11:18:17 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! I assume that you tested it, right? :)

tools/python/maps_generator/utils/file.py

biodranik (Migrated from github.com) commented

2022-09-10 11:15:41 +00:00

Should expanduser be called only if the path starts with ~?

tools/python/maps_generator/utils/file.py

biodranik (Migrated from github.com) commented

2022-09-10 11:16:07 +00:00

        # url uses 'file://' scheme

```suggestion # url uses 'file://' scheme ```

biodranik (Migrated from github.com) commented

2022-09-10 11:16:52 +00:00

Is there a rsync analog in python to avoid unnecessary copies when files are already in place?

Is there a `rsync` analog in python to avoid unnecessary copies when files are already in place?

biodranik (Migrated from github.com) commented

2022-09-10 11:17:33 +00:00

Why do you need a try-except if an exception is propagated anyway? Let's remove it )

mrater (Migrated from github.com) reviewed 2022-09-12 09:21:16 +00:00

tools/python/maps_generator/utils/file.py

mrater (Migrated from github.com) commented

2022-09-12 09:21:16 +00:00

There's something called pyrsync. As its pypi site describes: It is not a wrapper for rsync, but a set of functions which apply full rsync functionality through Python. So I think it might be a good equivalent.

There's something called pyrsync. As [its pypi site describes](https://pypi.org/project/pyrsync/): _It is not a wrapper for rsync, but a set of functions which apply full rsync functionality through Python_. So I think it might be a good equivalent.

mrater (Migrated from github.com) reviewed 2022-09-13 16:34:52 +00:00

tools/python/maps_generator/utils/file.py

mrater (Migrated from github.com) commented

2022-09-13 16:34:52 +00:00

Sure, it doesn't need to be called in every case.

mrater commented

2022-09-13 16:52:11 +00:00

(Migrated from github.com)

Thanks! I assume that you tested it, right? :)

Hello, yes, I tested it. I think most of your suggestions can be considered resolved except for which python equivalent of rsync do we want to use.

> Thanks! I assume that you tested it, right? :) Hello, yes, I tested it. I think most of your suggestions can be considered resolved except for which python equivalent of rsync do we want to use.

biodranik (Migrated from github.com) approved these changes 2022-09-13 20:24:04 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! What is your main use case for this change?

tools/python/maps_generator/utils/file.py

biodranik (Migrated from github.com) commented

2022-09-13 20:21:23 +00:00

Why extra line here?