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

new parser writes datalogs #4287

Closed
wants to merge 67 commits into from
Closed

Conversation

mck1117
Copy link
Member

@mck1117 mck1117 commented Jun 26, 2022

#4441 let's move forward
fixes #4451

@mck1117 mck1117 marked this pull request as ready for review August 16, 2022 00:00
@rusefillc
Copy link
Contributor

My focus right now is on ae8eb30

Copy link
Contributor

@rusefillc rusefillc left a comment

Choose a reason for hiding this comment

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

Please extract output_channels.txt change into separate PR

Copy link
Contributor

@rusefillc rusefillc left a comment

Choose a reason for hiding this comment

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

I believe that this PR should be removing TsOutput

@mck1117
Copy link
Member Author

mck1117 commented Aug 16, 2022

My focus right now is on ae8eb30

Yes, that should be pretty easy shortly.

@mck1117
Copy link
Member Author

mck1117 commented Aug 16, 2022

I believe that this PR should be removing TsOutput

that's still in use by the normal config generation, but not by live data

@rusefillc
Copy link
Contributor

I am failing to see the value of this change without TsOutput removal - as it it's introducing descepancy between two extremely similar tasks. I believe that TsOutput removal is the bare minimal of newparse if you choose to go with live data as first newparse usage.

@mck1117
Copy link
Member Author

mck1117 commented Aug 16, 2022

I don't understand your question. The live data tool as modified in this PR makes no call to TsOutput, but gen_config.sh does, so it isn't removed yet.

DataLogConsumer, OutputsSectionConsumer, and the constructor arguments to TsOutput are now removed since those were now dead code. There is nothing more to remove.

@rusefillc
Copy link
Contributor

my understaidnig of TsOutput constructor usages in master is "OutputsSectionConsumer and TSProjectConsumer are extremely similar"

my common sense is "if we replace OutputsSectionConsumer same PR should also be replacing TSProjectConsumer "

my common sense is "if we remove OutputsSectionConsumer same PR should also be removeing TSProjectConsumer "

Q: does this PR treat OutputsSectionConsumer and TSProjectConsumer the same way by migrating those together?

@rusefillc
Copy link
Contributor

do we want to change random things, or seek some stability in the world?

@mck1117 mck1117 mentioned this pull request Aug 17, 2022
@rusefillc
Copy link
Contributor

@mi-hol here we have a great idea which is I. The works for a couple of years and in my opinion the transition period of extra effort to maintain old and new is not justified

@mck1117 I still believe that we should either completely migrate to newparse or officially give up. What do you think @mck1117?

@mi-hol
Copy link
Contributor

mi-hol commented Apr 2, 2023

@mi-hol here we have a great idea which is I. The works for a couple of years and in my opinion the transition period of extra effort to maintain old and new is not justified

@rusefillc apologize but I fail to understand what you try to tell me. What I see is a harsh comment discouraging further progress on this matter. It's your choice how this will end up. From my view a balance between requirements of developer and owner are required to progress as a team.

@rusefillc
Copy link
Contributor

Abandoned

@rusefillc rusefillc closed this Apr 28, 2023
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.

invalid ini
3 participants