Skip to content
This repository has been archived by the owner on Feb 20, 2021. It is now read-only.

Added exception details #2

Merged
merged 3 commits into from May 3, 2016
Merged

Added exception details #2

merged 3 commits into from May 3, 2016

Conversation

tadams1138
Copy link
Contributor

Added exception details when a ReflectionTypeLoadException is thrown in SemanticLogging-svc.exe in console mode.

This saves many hours of debugging versioning issues with libraries referenced by the semantic logging out of process service and libraries referenced in the configuration.

@azurecla
Copy link

Hi @tadams1138, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, AZPRBOT;

@bennage
Copy link
Member

bennage commented Feb 19, 2015

Thanks. We're backed up right now, but we'll review this soon.

@mekoda
Copy link
Collaborator

mekoda commented Aug 21, 2015

Hi @tadams1138, thanks for the contribution. Could you add this exception details also in the other mode?

@tadams1138
Copy link
Contributor Author

I will try to do that tomorrow.
Thanks,Tom Adams

Date: Fri, 21 Aug 2015 10:11:42 -0700
From: notifications@github.com
To: semantic-logging@noreply.github.com
CC: tadams1138@hotmail.com
Subject: Re: [semantic-logging] Added exception details (#2)

Hi @tadams1138, thanks for the contribution. Could you add this exception details also in the other mode?


Reply to this email directly or view it on GitHub.

@tadams1138
Copy link
Contributor Author

I just finished adding code to log loader exceptions in both modes

@mekoda
Copy link
Collaborator

mekoda commented Sep 15, 2015

Thanks @tadams1138, the contribution is ok. One thing about the Git flow: seems it haves a merge commit; could you rebase your changes with the current state in the master branch? This way the merge commit is avoided.

Just as a suggestion and given the upcoming changes, you can amend your committer name in commit 8ffb27e, consolidating it with your other commits name.

After the rebase, the changes will be ready to be merged.

@tadams1138
Copy link
Contributor Author

I think I have it now just as you requested. Please let me know if that works.

@@ -199,6 +196,14 @@ private void DisplayExceptionOnConsole(Exception e)
this.ExitCode = ApplicationExitCode.RuntimeError;
}

private static void DisplayLoaderExceptionsOnConsole(ReflectionTypeLoadException e)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modifier can be non static.

@mekoda
Copy link
Collaborator

mekoda commented Mar 31, 2016

Hi @tadams1138,

Sorry for the late reply and thanks in advance for the contribution. Only a few things to consider before merging the pull request:

  • There's a method with an unnecessary static modifier (I commented it in the PR).
  • Can you rebase the PR with the latest changes in the master branch? There are 5 new commits there.

Thanks!

@tadams1138
Copy link
Contributor Author

Is there anything else I should do, or are you able to pull using my previous pull request?

@arambarri
Copy link
Collaborator

Hi @tadams1138,

Thanks for your time and patience.

I'm taking over the tasks from @mekoda

The code is working properly and looks ready.

Your branch looks like:

image

(your changes are added twice, once in each branch and merge after that)

And it should look like:

image

You will be able to get this state doing

We would be grateful if you could do these modifications and it will be ready to merge.

P.S. Today, if you take a look to "File Changed" tab, you will see files which you are not modified.

@tadams1138
Copy link
Contributor Author

Thanks for walking me through this. Git is not the most intuitive source control. How about now?

@manikrish manikrish merged commit 1a1713c into microsoftarchive:master May 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants