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 plist to plaintext formatting (parse) #1334

Merged
merged 1 commit into from
Feb 2, 2018

Conversation

gyorb
Copy link
Contributor

@gyorb gyorb commented Jan 29, 2018

Plist parsing and formatting used by the old architecture is not
the way it should be handled now. It was not flexible and it was hard
to improve it.
It should be easier to test and setup the output formatting after the
changes.

Skip lists were not supported by the parse command, this
commit will add the skip files support for parse too.

@gyorb gyorb force-pushed the ref-parse branch 2 times, most recently from dec4be4 to d290f48 Compare January 29, 2018 15:17
@whisperity whisperity added the CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands label Jan 29, 2018
Copy link
Contributor

@whisperity whisperity left a comment

Choose a reason for hiding this comment

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

One minor comment, rest looks good.

suppress_data = sp_handler.get_suppressed()
if suppress_data:
if self.suppress_handler:
LOG.info("Writing source-code suppress at '{0}:{1}' to "
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this output true? This should only be printed if --export-suppress is given to parse. What happens if a suppress file is given but we don't want to export into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a suppress file is given than the report will be printed by parse only if it is not suppressed in the suppress file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that. What I meant is the log output here if it holds in execution. I feel this log shouldn't be here.

Copy link
Contributor Author

@gyorb gyorb Feb 2, 2018

Choose a reason for hiding this comment

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

I've got your point. I've checked again and removed the log output. It was printed even if the suppress information was not written into a suppress file.

@gyorb gyorb force-pushed the ref-parse branch 2 times, most recently from cc8c1e3 to 7baae7f Compare February 2, 2018 09:19
@gyorb gyorb requested a review from whisperity February 2, 2018 10:10
Copy link
Contributor

@whisperity whisperity left a comment

Choose a reason for hiding this comment

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

docs/user_guide.md should be extended with the --skipfile argument for parse's documentation.

Otherwise OK.

@gyorb
Copy link
Contributor Author

gyorb commented Feb 2, 2018

Done.

Plist parsing and formatting used by the old architecture is not
the way it should be handled now. It was not flexible and it was hard
to improve it.
It should be easier to test and setup the output formatting after the
changes.

Skip lists were not supported by the parse command, this
commit will add the skip files support for parse too.
@csordasmarton csordasmarton added this to the release 6.5 milestone Feb 2, 2018
@csordasmarton csordasmarton merged commit a6f8a01 into Ericsson:master Feb 2, 2018
@gyorb gyorb deleted the ref-parse branch May 18, 2018 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands enhancement 🌟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants