Skip to content

Typecheck xmllint with ament_mypy#568

Draft
InvincibleRMC wants to merge 1 commit intoament:rollingfrom
InvincibleRMC:xmllint-mypy
Draft

Typecheck xmllint with ament_mypy#568
InvincibleRMC wants to merge 1 commit intoament:rollingfrom
InvincibleRMC:xmllint-mypy

Conversation

@InvincibleRMC
Copy link
Copy Markdown
Contributor

@InvincibleRMC InvincibleRMC commented Jan 23, 2026

Add ament_mypy type checking to ament_xmllint.

Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
@InvincibleRMC
Copy link
Copy Markdown
Contributor Author

InvincibleRMC commented Jan 23, 2026

Hmm it seems colcon cannot resolve a build tree since ament_mypy and ament_xmllint are test_depends of each other. Looking at colcon there seems to be an open issue covering a very similar situation except here it is a little more narrow in scope focused only on test_depend and that issue includes exec_depend. Looking at the other linters it seems we just lint in a tree where flake8 only lints itself to avoid this circular build limitation. I guess type checking the other linters with ament_mypy will have to wait.



def pytest_configure(config):
from _pytest.config import Config
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should be importing from a private module.

Suggested change
from _pytest.config import Config
from pytest import Config

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Following the pattern in other files, only a single newline between copyright and imports.

Suggested change

import time
from typing import Any
from typing import Literal
from typing_extensions import TypeAlias
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will effectively become a new dependency for all of ROS 2. Is there any way to avoid it?

@InvincibleRMC InvincibleRMC marked this pull request as draft February 2, 2026 21:17
@InvincibleRMC
Copy link
Copy Markdown
Contributor Author

@cottsay Thanks for the feedback unfortunately due to this linters can't lint other linters.

@cottsay
Copy link
Copy Markdown
Contributor

cottsay commented Feb 2, 2026

Thanks for the feedback unfortunately ... linters can't lint other linters.

I see. While I think there's an acute risk of this issue manifesting with linters-on-linters, I don't think this statement is universally true. That said, this change would create a cyclic dependency under the current interpretation of dependency scopes.

How would you like to move forward with this PR?

@InvincibleRMC
Copy link
Copy Markdown
Contributor Author

I think leaving in draft is probably best since this would be a good feature if the definition of cyclic dependencies ever gets updated. If you would prefer closing and possible future re-opening is better I would be fine with that.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants