AutoColumnize throws an Exception with multiple files if no bookmarks are found #102

[miltob]

Update the AutoColumnizer logic to fix the bug.

[zarunbal]

Just a question which did come up by a quick view (really quick, could be that I missed something).

• You directly implemented the ColumnizerManager into LogExpert?
• The IAutoColumnizer interface is then obsolete?
• Would it be possible to extract this as interface so its possible to replace the implementation easily or to disable it completly
• I'm not sure if its good to "deeply" integrate the ColumnizerManager into LogExpert. Maybe if it worked seamless for some releases but not direct from fresh. I just want to keep the base "stable" where it is (Looking back it wasn't a good idea to directly integrate the Autocolumnizer)

[miltob]

hi @zarunbal,
Yes, I splitted the AutoColumnizer into two parts. Hence IAutoColumnizer is not needed any more.
To me, the decision of search/replace columnizers is a core functionality which better stay in the LogExpert project.
I see what's your concern. I understand it's a high risk change and you'd like to make sure it's not break anything. I think we shouldn't afraid to change things simply because it's risky. But I agree we need more automated testing to cover us. I am glad to try implementing more tests together with this change. Maybe we could think about have some experimental release for something like this? When time comes then we merge it into formal release(master)? One way to do this is: not merge Development branch into master when releasing. Instead, each change comes with 2 PR(one to Development branch one to master). Only when we are happy, then merge the one to master branch.
However, if you think ColumnizerManager logically shouldn't be in LogExpert, then that's different.
Let me know what you think.

[miltob]

About the interfacing, yes, I can extract it as an interface. I was not think of it would need to be replaced.
I think ColumnizerManager as the place we initialize columnizers which is needed as always.
It does make sense if we are(or planning to) introducing DI container to this project. I feel like DI container is something quite handy here.

[zarunbal]

Normaly I would fully support the idea "change and don't be afraid" but this is in my current opinion not possible. As you said we are lacking automated tests and this is one of the major pain points which have to be changed (with changes in the architecture). Currently a release is for me quiet some work because I test it manually every time and hope I didn't miss anything.

We definitive introduce a DI container! I only wait until the project is switched to .NET 4.7.2. I plan to switch it soon (After the next release, which I plan in some days). Because most of the good container do not support .NET 4.0 any more. After the introduction of a DI Container the splitting up of the base can start.

I should read me in into the project abilities of GitHub and start using it.

[miltob]

@zarunbal I have added some more test cases and restructured a bit for the columnizer manager.
I have also done some manual test locally. The change works fine here. I will add more test once DI container is ready.
Please let me know if you think we need any change around this.
Also, when introducing DI container or any other priority tasks that you think I could help, please feel free to let me know. I am happy to help out as well.

[Hirogen]

Please don't use Manager as wording or try to avoid it as much as possible!

Nobody knows what a manager does or is doing! It can do everything or nothing
https://stackoverflow.com/questions/1866794/naming-classes-how-to-avoid-calling-everything-a-whatevermanager
https://blog.codinghorror.com/i-shall-call-it-somethingmanager/

I know it's been part of Logexpert a while, but for the future maybe :)

[zarunbal]

@Hirogen, great input thank you. I'll change this naming (as soon as I have a better name ;) ). This changed my naming style or added another consideration!

[zarunbal]

@miltob thank you for your ongoing help! If you find a issue that you think you can fix feel free. I think for the near future (After the introduction of DI) there are a bunch of tasks that can be done. As said I will try to use the github projects functionality and plan some tasks.

[zarunbal]

My name suggestion would be ColumnizerPicker what do you think about this name

[miltob]

Renamed ColumnizerManager to ColumnizerPicker.

[miltob]

Using Milestones or something similar would be great for prioritize tasks.

[zarunbal]

It seems to be a bug there. If you drop a file which contains xml and has a file extension xml. The LogExpert.Log4jXmlColumnizer has to be the current columnizer (or some other TimeSpread enabled columnizer). I just used the generated ColumnizerLib.XML in the bin/Debug folder.

Then there is a System.InvalidOperationException: 'Invoke or BeginInvoke cannot be called on a control until the window handle has been created.' in line.
I can only reproduce this error in the development branch so I assume this is related to the merged pull.

@miltob could you please have a look into it?

I hope this text is to a extend understandable I'm nearly sleeping on my keyboard ... gn8

[zarunbal]

I think the CurrentColumnizer is to early changed from the DefaultLogfileColumnizer to the matching columnizer.

[miltob]

I'll have a look. No worries.

[miltob]

@zarunbal Thanks for point out the issue. I have fixed the problem and added one test in a PR. The new added test case failed before the change and passed after the change. There are two things I'd like to talk about here besides the fix.

1. Doing UI related operation(E.g. Control.Invoke) in the constructor of a Control is not a good practice. I saw a lot of these behaviors happening as for now. We could do better in the future.
2. This test case created by thomas failed on my machine. I am not sure if that's my setup. Let me know if it fails on your machine as well. I can fix it in another PR.

[zarunbal]

Doing UI related operation(E.g. Control.Invoke) in the constructor of a Control is not a good practice. I saw a lot of these behaviors happening as for now. We could do better in the future.

Yes, I fully agree with you.

I started to integrate AutoFac as DI container into LogExpert. As soon it works well enough that we can work together, we could start transforming the codebase.

This test case created by thomas failed on my machine. I am not sure if that's my setup. Let me know if it fails on your machine as well. I can fix it in another PR.

I thought that I removed this Test (the "en-za"). Because this test depends on the executing OS. On my local machine this test fails too. I changed it to work on the appveyor environment.

[Hirogen]

@zarunbal since this is already merged, can this be set to "done" in the "next release" project?