sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

while doing Send/Receive using the new 2.6 Chorus from a repo that wa… #176

Closed bobeaton closed 5 years ago

bobeaton commented 6 years ago

…s last sync'd with 2.5, we occasionally get an exception, "Failed to set up extensions for the repository: Unable to remove the file to be replaced" (an exception throw from line 174 of HgRepository.cs). Jason suggests changing File.Replace to RobustFile.Replace in the Save method of IniDocument to solve this.

Also, OneStory Editor has ChorusFileType extension DLL that get loaded at run-time and 2.6 has a new behavior that requires the extension to define the class attribute: [Export(typeof(IChorusFileTypeHandler))] in the FileType handler. Before I knew this was needed, it failed to load the extension DLL and the error was being swallowed, so I added a try...catch around where it fails, so it can throw a more meaningful error message if this wasn't done.


This change is Reviewable

bobeaton commented 6 years ago

There was a change in the way external implementers of the IChorusFileTypeHandler where handled. Starting around 2.5 I think, the new way of incorporating them required the implementers to include an Attribute on their class implementing IChorusFileTypeHandler. But without that attribute, Chorus was failing silently with no explanation. So without wanting to interfere (in case there was a reason this should fail silently), I only meant to put in exception handling for that one case.

But yes, it would have been appropriate to just catch the one. (I might have been thinking to throw all exceptions and chickened out later).

Bob

From: Eberhard Beilharz [mailto:notifications@github.com] Sent: Wednesday, July 11, 2018 4:48 PM To: sillsdev/chorus chorus@noreply.github.com Cc: Bob Eaton beaton@sheplers.com; Author author@noreply.github.com Subject: Re: [sillsdev/chorus] while doing Send/Receive using the new 2.6 Chorus from a repo that wa… (#176)

@ermshiperete commented on this pull request.


In src/LibChorus/FileTypeHandlers/ChorusFileTypeHandlerCollection.cshttps://github.com/sillsdev/chorus/pull/176#discussion_r201850382:

@@ -44,8 +44,21 @@ public class ChorusFileTypeHandlerCollection

                           using (var container = new CompositionContainer(catalog))

                           {

Wouldn't it be better to directly catch the ReflectionTypeLoadException instead of all exceptions?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/sillsdev/chorus/pull/176#pullrequestreview-136434980, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEdMdXczAhIdapVPdlXvHVjIiFQ8LVUTks5uFnJ0gaJpZM4UpeMS.

This email has been scanned for email related threats and delivered safely by Mimecast. For more information please visit http://www.mimecast.com

bobeaton commented 5 years ago

One tiny comment, otherwise :lgtm:

Reviewed 2 of 2 files at r1, 2 of 2 files at r2. Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bobeaton)

src/LibChorus/FileTypeHandlers/ChorusFileTypeHandlerCollection.cs, line 53 at r2 (raw file):

                    catch (ReflectionTypeLoadException ex)
                    {
                        var typeLoadException = ex as ReflectionTypeLoadException;

No need to cast to ReflectionTypeLoadException anymore

sorry, didn't get an email about this.. just checked in the new change removing this.

Let me know when this is merge,

Thanks,Bob