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

Strip ANSI escape sequences when ansi2html is missing #315

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

BeyondEvil
Copy link
Contributor

Fixes: #314

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

I have mixed feelings regarding this because it goes a dangerous path (auto-processing input, not even mentioning why is stipping ANSI.)

Maybe we should log that we remove ANSI as ansi2html is not present.

Adding E203 as general exception is not acceptable. Inline exclude would be ok if not avoidable other way and if added as general exception, it must be preceded with a comment line explaining the reasoning for doing it. The only good reason I remember was incompatibility with black.

Also please rephrase it as "Strip ANSI when ansi2html is missing", it would be easier to understand why this needs to happen.

tox.ini Outdated
@@ -22,6 +22,7 @@ deps = pre-commit
commands = pre-commit run --all-files --show-diff-on-failure

[flake8]
ignore = E203
Copy link
Member

Choose a reason for hiding this comment

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

I see not reason for this, also no explanation. If is a bug in flake8, why not using an inline exclude instead of diabling an entire rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik, the rule is broken full stop, not in particular cases. It's also not PEP8 compliant.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, in that case just a comment similar to the one I proposed. Reasoning behind is that I want to discourage someone from adding arbitrary excludes in the future. Documenting reason would also avoid questions or the need to use blame to find why.

@BeyondEvil
Copy link
Contributor Author

BeyondEvil commented Jul 16, 2020

I have mixed feelings regarding this because it goes a dangerous path (auto-processing input, not even mentioning why is stipping ANSI.)

Maybe we should log that we remove ANSI as ansi2html is not present.

Why do you figure? To me this is an obvious bug, no? I think emitting a warning in this case would just add noise.

Adding E203 as general exception is not acceptable. Inline exclude would be ok if not avoidable other way and if added as general exception, it must be preceded with a comment line explaining the reasoning for doing it. The only good reason I remember was incompatibility with black.

Apparently E203 is broken and not PEP8 compliant, so I don't see any reason to not disable it completely. Especially since black seems to handle it correctly.

Reasoning from black

Even pytest itself ignores it.

Also please rephrase it as "Strip ANSI when ansi2html is missing", it would be easier to understand why this needs to happen.

You mean rephrase the PR?

@ssbarnea
Copy link
Member

You mean rephrase the PR?

Yeah. Usually I would have rephrased the PR myself but I do not have this privilege. As these end-up as release notes, I do not want to force user to raise a brow when reading it.

Regarding printing a warning, is was only a suggestion, not a request. It is very common for new users of pytest-html to miss having ansi2html, as is not an implicit requirement. Printing something, could make them fix the issue instead of wondering around.

@BeyondEvil
Copy link
Contributor Author

You mean rephrase the PR?

Yeah. Usually I would have rephrased the PR myself but I do not have this privilege.

No problem, I'll rephrase.

As these end-up as release notes, I do not want to force user to raise a brow when reading it.

Not sure what you mean, but there's no automation surrounding the release notes (yet). It's all manually written.

Regarding printing a warning, is was only a suggestion, not a request. It is very common for new users of pytest-html to miss having ansi2html, as is not an implicit requirement. Printing something, could make them fix the issue instead of wondering around.

Oh, I see. 🤔 Do you mean that the default usage of the plugin should be to use it in conjunction with ansi2html?

If that's the case, we could emit a more general warning saying that it's recommended to use it together with the plugin. @ssbarnea

@BeyondEvil BeyondEvil changed the title Remove ANSI escape sequences from HTML report Strip ANSI escape sequences from HTML report when ansi2html is missing Jul 16, 2020
@BeyondEvil BeyondEvil changed the title Strip ANSI escape sequences from HTML report when ansi2html is missing Strip ANSI escape sequences when ansi2html is missing Jul 16, 2020
@BeyondEvil BeyondEvil force-pushed the fix-ansi-in-report branch from a04e63a to 3a4efa1 Compare July 16, 2020 17:16
tox.ini Outdated
@@ -22,6 +22,7 @@ deps = pre-commit
commands = pre-commit run --all-files --show-diff-on-failure

[flake8]
ignore = E203
Copy link
Member

Choose a reason for hiding this comment

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

Perfect, in that case just a comment similar to the one I proposed. Reasoning behind is that I want to discourage someone from adding arbitrary excludes in the future. Documenting reason would also avoid questions or the need to use blame to find why.

Comment on lines +27 to +29
# rationale here:
# https://github.com/psf/black/blob/master/docs/the_black_code_style.md#slices
extend-ignore = E203
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @ssbarnea

Copy link
Member

Choose a reason for hiding this comment

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

I already approved but apparently I do not get enough rights for the vote to matter (is gray, not green).

@nicoddemus
Copy link
Member

Do you mean that the default usage of the plugin should be to use it in conjunction with ansi2html?

How about making ansi2html an explicit requirement?

@BeyondEvil
Copy link
Contributor Author

BeyondEvil commented Jul 16, 2020

Do you mean that the default usage of the plugin should be to use it in conjunction with ansi2html?

How about making ansi2html an explicit requirement?

Has something changed re. licensing?

@nicoddemus

@nicoddemus
Copy link
Member

Oh forgot about that issue, thanks for reminding me.

@nicoddemus
Copy link
Member

A quick search turns up https://bitbucket.org/dhrrgn/ansiconv, but I'm can't vouch if it is maintained.

@BeyondEvil
Copy link
Contributor Author

A quick search turns up https://bitbucket.org/dhrrgn/ansiconv, but I'm can't vouch if it is maintained.

Hasn't been touched since 2014.

Do you see an issue with my solution? It's straight from pytest itself.

@nicoddemus
Copy link
Member

Not in particular other than using a private function, otherwise LGTM!

@ssbarnea
Copy link
Member

TO be honest, I had an years old with to rewrite ansi2html with a MIT/BSD-like license that does not impose reusability limitations but never had time. The amount of code it has is very low. Still, I always have other more pressing issues to sort, maybe if we join efforts we can make it a reality?

I even bootstrapped it at https://github.com/pycontribs/a2h -- pypi package namespace too. With your help we can probably sort it quickly. There is a long list of python libraries that share the same issue around ansi2html, is not only pytest-html that would benefit from it.

@BeyondEvil
Copy link
Contributor Author

BeyondEvil commented Jul 17, 2020

TO be honest, I had an years old with to rewrite ansi2html with a MIT/BSD-like license that does not impose reusability limitations but never had time. The amount of code it has is very low. Still, I always have other more pressing issues to sort, maybe if we join efforts we can make it a reality?

I even bootstrapped it at https://github.com/pycontribs/a2h -- pypi package namespace too. With your help we can probably sort it quickly. There is a long list of python libraries that share the same issue around ansi2html, is not only pytest-html that would benefit from it.

I'm happy to help. But you would have to take lead. @ssbarnea

Also, this plugin needs a rewrite as well. Especially moving away from the py.xml dependency.

@ssbarnea
Copy link
Member

I sent invites to both of you. I will take case of pushing the code and setting up CI/CD pipelines for it. If I am lucky I may have something ready before Monday.

@ssbarnea
Copy link
Member

Sadly the plan to work on a2h over the weekend had to be postponed as I faced other more pressing fixing, like creating a maintenance fork for a ruamel.yaml library. Still, I will try to do something during next week.

@ssbarnea ssbarnea added the enhancement This issue/PR relates to a feature request. label Aug 10, 2020
@ssbarnea ssbarnea merged commit 992e3de into pytest-dev:master Aug 10, 2020
@BeyondEvil BeyondEvil deleted the fix-ansi-in-report branch August 13, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbage date-time is printed in the Captured log.
3 participants