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

line 537 is a noop in python3 #11870

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Conversation

Zectbumo
Copy link

@Zectbumo Zectbumo commented Jun 5, 2023

using map in python3 without iterating does nothing failing to register checkers.

Scope and purpose

Fixes #

Add a few words about why this PR is needed and what is its scope.
If the associate ticket(s) fully explain the need you can just refer to it/them.

Add any comments about trade-offs (if any) made in this PR and the reasoning behind them.

Add mentions of things that are not covered here and are planed to be done in separate PRs.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

  • The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
  • A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
  • The automated tests were updated.
  • Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
    Our bot will trigger the review process, by applying the pending review label
    and requesting a review from the Twisted dev team.

using map in python3 without iterating does nothing failing to register checkers.
@adiroiban
Copy link
Member

Thanks for looking into this. It looks like the current CI errors are not related to the changes from this PR.

The last CI run on trunk was green... it looks like this is another case of a test failure generated by un-pinned dependencies

@adiroiban
Copy link
Member

Thanks for the fix.
I have never used the Twisted MailService code before, but by reading the code, it makes sense and the fix looks good.

I can see that the test are now green.

From what I can see, the tests were green even before this fix... so it means that we don't have automated tests for this part.

Would it be possible to also update the automated tests to make use of the fix from here?

Thanks

@Zectbumo
Copy link
Author

From what I can see, the tests were green even before this fix... so it means that we don't have automated tests for this part.

Right, that's because most of the tests have been disabled due to python 3.

Would it be possible to also update the automated tests to make use of the fix from here?

I have made a larger set of modifications to make the mail portion python 3 compatible and tried to re enable several of the tests again. I was working through a good number of tests up until I ran out of time and couldn't revisit the code again. I think I was close to finishing except it was a bit of a can of worms. The more I opened the tests up the more worms that crawled out.

@adiroiban
Copy link
Member

Thanks for the follow-up.

If you managed to get a single test on Py3, I think that it is worth sending a patch for it.

It is much better than not running any test at all.

We don't need to get all the tests running from a single PR.


If the current mail service is not yet py3 ready and doesn't work on Py3, then I am not sure if this change will help in any way :)


I encourage you to push your current code/patch as it is.
Even if it's not 100% ready
If someone else would want to run the mail service on py3 , they can use it as a starting point.


I have never used the Twisted mail functionality before and I am not familiar with this part of Twisted.
I am just providing high level feedback

@Zectbumo
Copy link
Author

Good point. Yes, I have activated and passed several py3 tests already. I will commit as is then.

@Zectbumo
Copy link
Author

push your current code/patch as it is

done #11898

@glyph
Copy link
Member

glyph commented Feb 12, 2024

please review

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

Successfully merging this pull request may close these issues.

None yet

4 participants