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

python: Future-proof use-ftp-tls for stdlib libdefs #3457

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

nmote
Copy link
Contributor

@nmote nmote commented Aug 21, 2024

Explanation in the rule

Copy link
Contributor

@yosefAlsuhaibani yosefAlsuhaibani left a comment

Choose a reason for hiding this comment

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

Yeah this looks good: appreciate the in depth comment!

@nmote
Copy link
Contributor Author

nmote commented Aug 21, 2024

Thanks! I'll wait a bit to merge in case someone from the Code team wants to weigh in.

@inkz
Copy link
Member

inkz commented Aug 22, 2024

@nmote ftplib.FTP(...) pattern should always match only ftplib.FTP, adding exclusion for FTP_TLS would make sense if we had something like this:

  patterns:
    - pattern: ftplib.$WHATEVER(...)
    - pattern-not: ftplib.FTP_TLS(...)

I recommend adding test case with FTP_TLS first, then see how the rule performs and if updates needed add them

@kopecs
Copy link
Contributor

kopecs commented Aug 22, 2024

@inkz I think Nat is saying that in ftplib FTP_TLS subclasses FTP, and we have the pattern A match B when A is a subclass of B. Previously we didn't know that FTP_TLS inherited from FTP, but we now do/will know with interfile.

@nmote
Copy link
Contributor Author

nmote commented Aug 22, 2024

@inkz In fact there is such a test case, and it failed on the branch I'm using to develop Python libdefs, for the reason that @kopecs explained. That's why I put up this PR.

@nmote
Copy link
Contributor Author

nmote commented Aug 22, 2024

This is the test case that fails when this rule is run using libdefs:

# ok: use-ftp-tls
ftpc = ftplib.FTP_TLS("example.com", "user", "pass", context=ssl.create_default_context())

@nmote nmote merged commit 3c793a0 into develop Aug 22, 2024
8 checks passed
@nmote nmote deleted the nmote/ftp branch August 22, 2024 20:41
@inkz
Copy link
Member

inkz commented Aug 23, 2024

Interesting! thanks for clarification @nmote @kopecs
is information about ftplib classes hardcoded in the engine?

@nmote
Copy link
Contributor Author

nmote commented Aug 23, 2024

It will be for the interfile engine, just like we currently hardcode information about the Java standard library.

@inkz
Copy link
Member

inkz commented Aug 25, 2024

@nmote Cool! thanks

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

Successfully merging this pull request may close these issues.

4 participants