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

refactor: split dlt_common and dlt_log #483

Closed

Conversation

danielweber2018
Copy link
Contributor

Seperate all internal logging function into an own source file. This reduces the scope of the sprawling dlt_common source file.

The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0

Daniel Weber, daniel.w.weber@mercedes-benz.com, Mercedes-Benz Technology Innovation GmbH, imprint

@danielweber2018
Copy link
Contributor Author

danielweber2018 commented May 2, 2023

@michael-methner Here my refactor PR like discussed at #369 (comment)

@danielweber2018 danielweber2018 force-pushed the split_common_and_log branch from d318093 to 7620b87 Compare May 3, 2023 14:00
@minminlittleshrimp minminlittleshrimp self-assigned this Jul 11, 2023
@minminlittleshrimp
Copy link
Collaborator

minminlittleshrimp commented Jul 11, 2023

Hello @michael-methner
This refactor is good, however I think it needs carefully reviewing from DLT team and considering upscale to be new release.
How do you think we could make to process this PR?

@danielweber2018
Copy link
Contributor Author

Hello @michael-methner ,
this MR has been open for 6 months today. I know it's a big change, and you probably have little time.
Nevertheless, I would greatly appreciate a review or another reaction.

Thank you in advance!

@danielweber2018
Copy link
Contributor Author

Hello @michael-methner ,
this PR is now over one year old. Some things are now outdated and I have also found some mistakes/conflicts in the code.
If you would like to review this PR, please let me know so I can update the PR.
If I don't hear from you, I'll assume there won't be a review and therefore won't invest the time in maintaining this PR.
If this PR is thematically undesirable, you can also close it.

@michael-methner
Copy link
Collaborator

Hello @danielweber2018 ,

thanks for your patience. I had a quick glance at the commit and I think is fine and a desirable change. I think it's good to spent some efforts on refactoring to keep the DLT code maintainable.

So please go ahead with your planned rework. I will then review it. I guess reviewing it is not that much of an effort as it is mainly moving code and and not modifying the logic of it.

@danielweber2018
Copy link
Contributor Author

Hello @michael-methner ,
thank you very much for the reponse, I really appreciate it :)
In this case, I will update this PR soon and then get back to you.

Seperate all internal logging function into an own source file.
This reduces the scope of the sprawling dlt_common source file.
@danielweber2018
Copy link
Contributor Author

Hello @michael-methner and @minminlittleshrimp ,
i updated and maintained my PR. I compared the moved code with the current master. So the PR is ready for review and i would be very happy about a soon response.

@danielweber2018
Copy link
Contributor Author

Hello @michael-methner and @minminlittleshrimp ,
what is your schedule for this MR? I would definitely like to avoid this PR falling asleep again.

Copy link
Collaborator

@minminlittleshrimp minminlittleshrimp left a comment

Choose a reason for hiding this comment

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

I am ok with your change, it is a refactoring only and no APIs affected.
Approve and merge!

@minminlittleshrimp
Copy link
Collaborator

Seems to have merge conflict, let me take sometimes to fix

@minminlittleshrimp
Copy link
Collaborator

Hello @danielweber2018
Could you do the merge conflict resolving base on this #654?
Then I can process with merging this PR.
Thanks

@minminlittleshrimp
Copy link
Collaborator

No response from contributor, create new one & set this to duplicated. Close and continue on: #657

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

Successfully merging this pull request may close these issues.

3 participants