Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1465 remove warning #809

Merged
merged 14 commits into from
Jun 30, 2020
Merged

1465 remove warning #809

merged 14 commits into from
Jun 30, 2020

Conversation

abdelr
Copy link
Member

@abdelr abdelr commented Jun 18, 2020

No description provided.

@msevestre
Copy link
Member

@abdelr I do not understand why this code was removed from CORE as this is a service that should also be used in MoBi

@abdelr
Copy link
Member Author

abdelr commented Jun 18, 2020

@abdelr I do not understand why this code was removed from CORE as this is a service that should also be used in MoBi

As discussed in Open-Systems-Pharmacology/PK-Sim#1507 the code was duplicated in PKSim and we did not see any reference to it on MoBi. If we would like to use this in MoBi also then we should indeed move it to Core and remove it from PKSim. I would wait for the release though so we could build a new version of our nuget and use the changes in PKSim without any code duplication to avoid forgetting it again.

@msevestre
Copy link
Member

@abdelr now that the release is behing us, please add everything back to core as we discussed
(also renaming ILogger to IOSPLogger as suggested) .
The version will be 10.x.

Don't forget to rebase on develop

@abdelr
Copy link
Member Author

abdelr commented Jun 23, 2020

@msevestre Thanks for letting me know. I will start with the changes. One quick question. Did you come to an agreement about the FileLogger? Should we totally replace it by a third party implementation or should we fix the StreamWriter issue?

Apart from the logic regarding FileLogger, everything else should be done now.

georgeDaskalakis and others added 4 commits June 23, 2020 10:41
OSPSuite.Core.Services.ILogger renamed to IOSPLogger, to avoid confusion with Microsoft.Extensions.Logging.ILogger
@msevestre
Copy link
Member

@abdelr @georgeDaskalakis
As proposed last Friday, I'd like you to open the discussion tomorrow during our meeting to explain why we should move away from the current implementation. Adding 5+ new dependencies to replace a system that is under 50 lines of code including comments needs to be really thought through. More specifically, you need to explain what we would be able to do that we cannot do now with the very naive and simple implementation of the FileLogger.

@abdelr
Copy link
Member Author

abdelr commented Jun 25, 2020

@msevestre Core should be in its final version now. Please take a look and merge it to develop if you agree with all changes. Also, we need to remove Serilog references and LoggingBuilderExtensions from PKSim since this has been moved to Core. The problem is that the nuget version is a bit strange yet with the naming. Last version is currently 10.0.5-cuhhvhhx and we would need it to be just 10.0.0.5. We need to refine the process now so we could reset the last digit on the version so we could easily update our references in PKSim.

registerLogging(container);
}

private static void registerLogging(IContainer container)
Copy link
Member

Choose a reason for hiding this comment

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

yep. Well done


public ILogger GetOrCreateLogger(string categoryName)
{
var logger = _loggerDict.GetOrAdd(categoryName, (_) => SetupLogger(categoryName));
Copy link
Member

Choose a reason for hiding this comment

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

Small improvement. Return directly instead of creating a variable


private ILogger SetupLogger(string categoryName)
{
ILogger logger;
Copy link
Member

Choose a reason for hiding this comment

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

small improvement.
REturn directlu in the using instead of creating a variable outside and returning it

return logger;
}

private ILogger SetupLogger(string categoryName)
Copy link
Member

Choose a reason for hiding this comment

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

This does not follow our convention. Private method => camelCase => setupLogger

@msevestre
Copy link
Member

Sorry I had reviewed but forgot to submit my comments.
Just one required change because coding standard are not respected. Aside from that good to go

@abdelr
Copy link
Member Author

abdelr commented Jun 30, 2020

@msevestre Thanks! I have just committed the changes you requested.

@msevestre
Copy link
Member

Well done

@msevestre msevestre merged commit 8fefddf into develop Jun 30, 2020
@msevestre msevestre deleted the 1465_Remove_Warning branch June 30, 2020 12:23
@msevestre
Copy link
Member

Don't forget to reset the appveyor flags as discussed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants