Importing files in a more tolerant way

[gatl]

It is currently impossible to import files that are mixed with files that cannot be read by the current user. This patch will allow such situations to be handled gracefully and collect all the files that can be found, instead of throwing an exception and halting the process.

[Kalanyr]

Hi. Just wanted to say thank you for these and let you know I'm working on a code rewrite so it may be a while before they get merged.

…

On Sun, 5 Jan 2020, 5:50 am gatl, ***@***.***> wrote: It is currently impossible to import files that are mixed with files that cannot be read by the current user. This patch will allow such situations to be handled gracefully and collect all the files that can be found, instead of throwing an exception and halting the process. ------------------------------ You can view, comment on, or merge this pull request online at: #20 Commit Summary - Removed trailing whitespace. - Ensure a space after a comma - Remove double spaces from the code - Catch some exceptions when importing files File Changes - *M* gogrepo.py <https://github.com/Kalanyr/gogrepoc/pull/20/files#diff-0> (909) Patch Links: - https://github.com/Kalanyr/gogrepoc/pull/20.patch - https://github.com/Kalanyr/gogrepoc/pull/20.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#20?email_source=notifications&email_token=ABKZ335PDUOAK7BGE6RCK33Q4DR7JA5CNFSM4KCXT5J2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IEA776A>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKZ333OG4O4KYO5GD5LA2TQ4DR7JANCNFSM4KCXT5JQ> .

[gatl]

A rewrite? That is such a good news!

I made a list of things I would like to make different after taking a look at the code, but to implement them it would need a significant rewriting and before I could do that I would need to study the code in more detail just to ensure I would not break anything important. All that would add-up to about two months of work, just to future-proof the code.

As it stands, I got it to make what I needed, so all the rest is not so important, but I'm glad you are improving the code. If you don't mind, I'll leave a few thoughts for your consideration.

[Kalanyr]

Yes, please feel free, though if some of it is action items, it's probably best to put them in issues where definitely I'll see it when I'm thinking about stuff to do but I'm happy to hear general thoughts too.

…

On Sun, 5 Jan 2020, 8:29 pm gatl, ***@***.***> wrote: A rewrite? That is such a good news! I made a list of things I would like to make different after taking a look at the code, but to implement them it would need a significant rewriting and before I could do that I would need to study the code in more detail just to ensure I would not break anything important. All that would add-up to about two months of work, just to future-proof the code. As it stands, I got it to make what I *needed*, so all the rest is not so important, but I'm glad you are improving the code. If you don't mind, I'll leave a few thoughts for your consideration. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#20?email_source=notifications&email_token=ABKZ335EFIZ5W7P7T4VE6XLQ4GZCBA5CNFSM4KCXT5J2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIDTLFQ#issuecomment-570897814>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKZ335QILBPNGE5N7TPIILQ4GZCBANCNFSM4KCXT5JQ> .

[gatl]

(Since this project has no issue tracking enabled, I'll leave my list in short in here):

• Use JSON to store the manifest. This allows for other projects to read the manifest and closes the security hole that is the eval() being called on a data file.

• Is the resume manifest really necessary? If we store a JSON object with the list of all games at the top of the manifest, and then add one JSON object entry (e.g. line) per game, we can always know where we left off and proceed. If we want to start over, just delete the manifest file! I think it is pretty clean.

• When importing files, you don't need to compute the MD5 on all files. If the size of the file does not match the size of a file on your manifest, you can be pretty sure the MD5 won't match either. So filter for file size first and for MD5 second. You'll see big improvements on importing speed.

• Still when importing, if the file size and the file name are the same, and no MD5 information is provided by GOG, then I perhaps I could trust that the file is the same. I would like to see this option added to make it easy to move/change my repository around.

• Personally, I would consider dropping support for Python 2 (people who need it can continue to use the current version of gogrepo) to get a much cleaner, simpler and more modern codebase. Then the option of using Async IO could be put on the table. There would be no need to adjust the number of concurrent workers and the system would scale better with the bandwidth that is available. But that is just my opinion.

Thank you for your continuing effort. Hit me up when a new version is out or if you want want to discuss ideas with someone. :-)

[Kalanyr]

(Darn, thought that was on , I'll look into it) Most of those ideas seem good. I have some concerns about the lack of human readability for JSON , I've had to hand examine / edit the manifest before. I suppose a dump / load option is possible for that. The resume manifest is necessary for partial updates (particularly the combined update / new ) because GOG clears those flags when the data is fetched. It is possible to roll that into the normal manifest file but it brought up backwards compatibility issues at the time. In the case of a full update, it does seem possible to do implicit storage of the position though I can think of some edge cases.

…

On Mon, 6 Jan 2020, 5:10 am gatl, ***@***.***> wrote: (Since this project has no issue tracking enabled, I'll leave my list in short in here): - Use JSON to store the manifest. This allows for other projects to read the manifest and closes the security hole that is the eval() being called on a data file. - Is the *resume manifest* really necessary? If we store a JSON object with the list of all games at the top of the manifest, and then add one JSON object entry (e.g. line) per game, we can always know where we left off and proceed. If we want to start over, just delete the manifest file! I think it is pretty clean. - When importing files, you don't need to compute the MD5 on all files. If the size of the file does not match the size of a file on your manifest, you can be pretty sure the MD5 won't match either. So filter for file size first and for MD5 second. You'll see big improvements on importing speed. - Still when importing, if the file size and the file name are the same, and no MD5 information is provided by GOG, then I perhaps I could trust that the file is the same. I would like to see this option added to make it easy to move/change my repository around. - Personally, I would consider dropping support for Python 2 (people who need it can continue to use the current version of gogrepo) to get a much cleaner, simpler and more modern codebase. Then the option of using Async IO could be put on the table. There would be no need to adjust the number of concurrent workers and the system would scale better with the bandwidth that is available. But that is just my opinion. Thank you for your continuing effort. Hit me up when a new version is out or if you want want to discuss ideas with someone. :-) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#20?email_source=notifications&email_token=ABKZ336LSVF6A5L4SK4ZFNTQ4IWB5A5CNFSM4KCXT5J2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEID5JPY#issuecomment-570938559>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKZ33ZPI3IGRJKGZLQZ3FLQ4IWB5ANCNFSM4KCXT5JQ> .