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

[PyQGIS] Add message to /Deprecated/ annotations #59834

Merged
merged 8 commits into from
Dec 17, 2024

Conversation

troopa81
Copy link
Contributor

This PR implements the QGIS part of QEP #287

It follows Python-SIP pull request #46 which allows to generate @deprecated decorator instruction in pyi files and support deprecated annotation with message. This new feature is only available in SIP 6.9.0.

As a result, if a python plugin use the deprecated function QgsProject.fileInfo() for instance,

When running, you'd get the following (same as before but with the \deprecated message)

/home/julien/work/QGIS/.worktrees/sip_deprecated/./deprecated_qgis.py:4: DeprecationWarning: QgsProject.fileInfo() is deprecated: Since 3.2. Use absoluteFilePath(), baseName() or lastModifiedTime() instead.
  p.fileInfo()

With using static Python linter pyright, you'd get the following

  /home/julien/work/QGIS/.worktrees/sip_deprecated/deprecated_qgis.py:4:3 - error: The method "fileInfo" in class "QgsProject" is deprecated
    Since 3.2. Use absoluteFilePath(), baseName() or lastModifiedTime() instead. (reportDeprecated)
1 error, 0 warnings, 0 informations 
pyright configuration file pyproject.toml
[tool.pyright]
include = ["deprecated_qgis.py"]
reportDeprecated = true

It's not possible to activate the deprecated message only for version greater or equal than SIP 6.9.0 using %if directive (with either %Feature or SIP range version) because of this issue fixed now (but would fail with all versions below 6.9.0).

So the only remaining option was to have a separate folder like we did for Qt6. This would lead to even more sip files, so I prefer not to. One solution would be, maybe, to generate those files at packaging time, before building.

This PR proposes also to:

  • fail sipify_all.sh properly is sipify.py failed
  • raise an error if SIP_DEPRECATED is used but no \deprecated instruction has been set in doxygen documentation
  • fix missing \deprecated instruction
  • Fix build without QSci Sip

@troopa81 troopa81 added API API improvement only, no visible user interface changes PyQGIS Related to the PyQGIS API labels Dec 11, 2024
@github-actions github-actions bot added this to the 3.42.0 milestone Dec 11, 2024
@troopa81
Copy link
Contributor Author

troopa81 commented Dec 11, 2024

@m-kuhn the following method has been deprecated only in SIP not in C++ code : QgsGeometry::set( QgsAbstractGeometry *geometry SIP_TRANSFER ) here.

Reading the PR comments, I fail to see if it's still relevant, do you know if it still is?

I planned in this PR to completely forbid this behavior (deprecated in Python, not in C++) and this is the only one failing at the moment. Do I have to reconsider and allow this kind of behavior? Or make an exception for this one?

@nyalldawson
Copy link
Collaborator

I planned in this PR to completely forbid this behavior (deprecated in Python, not in C++)

I think this should be permitted, eg for cases where we accidentally exposed something we'd rather keep C++ only.

(But if it's too much trouble to support we could resort to marking the C++ one deprecated and then
use the nowarn deprecated push/pull macros throughout the C++ code, and then cleanup for 4.0)

@nyalldawson
Copy link
Collaborator

Regarding

It's not possible to activate the deprecated message only for version greater or equal than SIP 6.9.0 using %if directive (with either %Feature or SIP range version) because of this issue fixed now (but would fail with all versions below 6.9.0).

Couldn't we use some regex magic in cmake when processing the .sip.in files to remove / retain these based on a cmake sip version variable? (Or manually set Boolean variable)

Copy link

github-actions bot commented Dec 11, 2024

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 4eab531)

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 4eab531)

@troopa81
Copy link
Contributor Author

Couldn't we use some regex magic in cmake when processing the .sip.in files to remove / retain these based on a cmake sip version variable? (Or manually set Boolean variable)

I thought about that but the cmake configure() allows only to replace variables. But maybe we could create our own cmake command that replace variables and strip Deprecated message when the SIP version is lower than 6.9.0. I'm wondering if it isn't a bit ugly and fragile.

@troopa81
Copy link
Contributor Author

I think this should be permitted, eg for cases where we accidentally exposed something we'd rather keep C++ only.

Maybe add a SIP_PYTHON_ONLY_DEPRECATED to make the difference with all other ones where we want to check that Q_DECL_DEPRECATED, SIP_DEPRECATED and \deprecated are consistent

@3nids
Copy link
Member

3nids commented Dec 11, 2024

@troopa81 I had a very quick read and I might be wrong, but the conditional directive depending on the SIP version looks something I've done in the past.
See 1d4e6a8#diff-005a07ca82fb46b215f58a3a8f4d6d6cc6ca088ade44e8f15cc2e6a51d1da6f4R178
the whole PR: #6074

That's why we have .in files, so they can be configured depending on the SIP version.

Again, I am not sure this helps, but it's worth a try.

@troopa81
Copy link
Contributor Author

@nyalldawson and @3nids I fixed the sip version issue with some cmake magic, /deprecated/ is now pushed with message and this message is removed with cmake when sip is less than 6.9.0

I also removed the string requirement on \deprecated when SIP_DEPRECATED is used so QgsGeometry::set can stay the way it is

@nyalldawson
Copy link
Collaborator

LGTM, thanks!

@troopa81 troopa81 merged commit 62c6b91 into qgis:master Dec 17, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes PyQGIS Related to the PyQGIS API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants