Matmaus / LnkParse3

Windows Shortcut file (LNK) parser
MIT License
63 stars 13 forks source link

bug fixes #1

Closed ernix closed 3 years ago

ernix commented 3 years ago

You may want to tweak LnkParse3/__about__.py before upload to PyPI.

ernix commented 3 years ago

Thank you for your reply. 😄

TBH, I didn't intend to make this pull request at the very beginning as I couldn't find which upstreams it should be based on. I just wanted to correct the Unicode/UTF-16LE bug and parse old Japanese LNK files using this product. But through several refactoring I made, I think it's worth to ask your opinions for the sake of better patches.

Yes, this P-r is extremely massive/messy and have got to be squared away before merging. I would re-roll them. Before I organize patches, I'd like to ask your preferences of the package structure. I've never published my packages on PyPI and I've just followed https://github.com/pypa/sampleproject. There are also runner.py for runtime testing and classes are including many relative imports. This structure might be awkward for you. Would you tell me a direction how the package style should be?

The first thing that is bothering me is removing of support for not absolutely valid files, i.e. invalid modification_time or clsid (first present here). I would suggest adding at least an option to suppress (all) failures (try..catch, with contextlib.suppress, ...). For example in the case droid_volume_id does not exist, it will skip link_tracker_block, but all other information will be available. Motivation is to get all possible information even in the case of some corruption. To inform that result may not be correct, some corruption trait may be present. If you can/want to do it, I would be grateful. Otherwise I would do it after merge.

It seems like there are tons of corrupted LNK files in the wild that depart from MS-SHLLINK definition. The problem is to what extent we should allow the file corruptness. I was looking for something like "Corrupted LNK file samples", but no luck. If you don't mind, could you add corrupted files into tests directory?

Secondly, I noticed that you have declared yourself as an author and updated links to point into your fork (which is relevant if in your repo). Can you please restore the links to its original values? About the authors, I am not sure what to do in setup.py. It should be probably a creator name.. I saw, that it can be a string with multiple names also, but it is not expected to be... What do you think?

Yes, I can. I will split this P-r into several parts excluding changes of metadata. About setup.py, I think the author should be you as well, because LnkParse3 belongs to you too.

Thanks

Matmaus commented 3 years ago

Before I organize patches, I'd like to ask your preferences of the package structure. I've never published my packages on PyPI and I've just followed https://github.com/pypa/sampleproject. There are also runner.py for runtime testing and classes are including many relative imports. This structure might be awkward for you. Would you tell me a direction how the package style should be?

I am not a much-experienced person as well, and I have no problem with the way it is, but here are some suggestions:

It seems like there are tons of corrupted LNK files in the wild that depart from MS-SHLLINK definition. The problem is to what extent we should allow the file corruptness. I was looking for something like "Corrupted LNK file samples", but no luck. If you don't mind, could you add corrupted files into tests directory?

I can give you a few corrupted files as well as a few not corrupted (commit).

I would go with the way it was, i.e. when just some part of parsing fails e.g target, other parts may be still available. Also, an invalid date should not be a problem. See the samples I would provide, they are all processable by 0.3.3 version.

Matmaus commented 3 years ago

Thank you for the job you have done. I'll make some little changes soon, including README improvement, and then release a new version if everything seems fine.

Thanks @ernix :slightly_smiling_face: :+1:

ernix commented 3 years ago

@Matmaus I should thank you too 😄 It was a nice experience working with you. Really appreciated your time and effort.