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

Create rule S7156 #4491

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rules/S7156/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
23 changes: 23 additions & 0 deletions rules/S7156/python/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"title": "\"copy.replace\" should not be invoked with an unsupported argument",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant/Issue",
"constantCost": "5min"
},
"tags": [],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-7156",
"sqKey": "S7156",
"scope": "All",
"defaultQualityProfiles": ["Sonar way"],
"quickfix": "unknown",
"code": {
"impacts": {
"MAINTAINABILITY": "HIGH",
"RELIABILITY": "MEDIUM"
},
"attribute": "CONVENTIONAL"
}
}
78 changes: 78 additions & 0 deletions rules/S7156/python/rule.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
:object_replacement_protocol: https://docs.python.org/3/library/copy.html#object.__replace__

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually here we do give a small description. Something like this rule raises an issue when copy.replace is used on an incorrect type. It is optional but I think it is nice if we have rule that all have follow a similar structure.

== Why is this an issue?

Calling ``++copy.replace(...)++`` with an argument of an unsupported type will raise an ``++TypeError++``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a good idea to start by stating that this is for Python 3.13. i.e.: In Python 3.13 the copy.replace...

Types supported by ``++copy.replace(...)++`` must implement the {object_replacement_protocol}[replace protocol].
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good start, but you should write the rule thinking the person reading it sees .replace for the first time. Maybe glance a bit at the other rules, the numpy or pandas rules. Also you could see with the rules for Java they usually have a nicely detailed WHY.


The following built-in types are supported by ``++copy.replace(...)++``

* ``++collections.namedtuple()++``
* ``++dataclasses.dataclass++``
* ``++datetime.datetime++``, ``++datetime.date++``, ``++datetime.time++``
* ``++inspect.Signature++``, ``++inspect.Parameter++``
* ``++types.SimpleNamespace++``
* https://docs.python.org/3/reference/datamodel.html#code-objects[code objects]

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just add a section about exceptions saying that this rule only raises for Python 3.13.

== How to fix it

If the argument passed in is a class defined in this project then implementing the {object_replacement_protocol}[replace protocol] by defining the ``++__replace__++`` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Bear in mind that the section How to fix and Why is it an issue are on different tab. So it is best to treat them as if the user did not read the previous section. So here for example it would be better to go with If the argument passed to the replace method.... Also in this section you can be more assertive. Instead of then implementing... you could go with to fix the issue, implement the protocol...


[source,python,diff-id=1,diff-type=compliant]
----
class SomeClass:
def __init__(self, name)
self.name = name

def __replace__(self, /, **changes)
return SomeClass(changes.get("name", self.name))
----

=== Code examples

==== Noncompliant code example

[source,python,diff-id=1,diff-type=noncompliant]
----
import copy

class AClass:
...

a = AClass()
b = copy.replace(a) # Noncompliant
Copy link
Contributor

Choose a reason for hiding this comment

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

When commenting with # Noncompliant it would be good to also say why. So Noncompliant: the class A does not implement...

----

==== Compliant solution

[source,python,diff-id=1,diff-type=compliant]
----
import copy

class AClass:
...
def __replace__(self, /, **changes):
...

a = AClass()
b = copy.replace(a) # Compliant


@dataclass
class ADataClass:
...

c = ADataClass()
d = copy.replace(c) # Compliant
----

=== Pitfalls

Ensure that if the ``++__replace__++`` is implemented that the implementation creates a new object instead of updating the old one.


== Resources
=== Documentation
* https://docs.python.org/3/library/copy.html#copy.replace
Copy link
Contributor

Choose a reason for hiding this comment

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

You can find how to format these sections and links in the docs folder . There are also a few guidelines available in the docs of sonarsource

* https://docs.python.org/3/library/copy.html#object.\\__replace__
* https://docs.python.org/3/whatsnew/3.13.html#copy
Loading