Skip to content

Project_Scanner: PythonScanner fixes and testing - #197

Merged
pjljvandelaar merged 8 commits into
mainfrom
python-scanner
Sep 23, 2026
Merged

pjljvandelaar merged 8 commits into
mainfrom
python-scanner

Conversation

@FrancescoPezzella

@FrancescoPezzella FrancescoPezzella commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

No longer hard-locked to only look for src/lib/test directories.

Instead of having duplicate Scanner methods and old broken code, I decided to make a PR addressing this issue. To be clear, other methods like JavaScanner also have their own issues and that has been noted with a TODO comment.

(#198)

@FrancescoPezzella FrancescoPezzella self-assigned this Sep 18, 2026
@FrancescoPezzella FrancescoPezzella added bug Something isn't working infrastructure issues related to the infrastructure for building, testing, and distribution labels Sep 18, 2026
@FrancescoPezzella
FrancescoPezzella marked this pull request as ready for review September 18, 2026 15:20
@FrancescoPezzella
FrancescoPezzella added this pull request to stack #203 September 21, 2026 12:52
@FrancescoPezzella

Copy link
Copy Markdown
Collaborator Author

The CI/CD failing has nothing to do with this branch. When pymarkdown was updated to 0.9.40, it added new doc requirements.

@pjljvandelaar

Copy link
Copy Markdown
Collaborator

We will merge after #205
As tests should pass afterwards.

@pjljvandelaar

Copy link
Copy Markdown
Collaborator

Could you explain why you opted for using str instead of Path?

When using str don't you have the risk that it isn't a valid Path? If so, shouldn't an exception be captured and rethrown in that case - to ensure a clear and correctly localized exception to the user?

@FrancescoPezzella

Copy link
Copy Markdown
Collaborator Author

Could you explain why you opted for using str instead of Path?

I did it for the sake of consistency. Changing it to Path would have been a bigger change, since other methods don't use it. Find_Source methods in all the other Scanners return a list[str]. I didn't want PythonScanner to be exception.


    def find_sources(self) -> list[str]:
        """Return every file entry listed in compile_commands.json, sorted and deduplicated."""
        if not Path(self.compile_commands_path).exists():
            message = "compile_commands.json not found"
            raise FileNotFoundError(message)
        with Path(self.compile_commands_path).open() as f:
            commands = json.load(f)
        return sorted({entry["file"] for entry in commands if "file" in entry})
    def find_sources(self) -> list[str]:
        """Return every .java file under root_dir, sorted."""
        # TODO: is this correct? does this filter out files correctly?
        # See https://github.com/TNO/Renaissance.Py/issues/200

        java_files = Path(self.root_dir).rglob("*.java")
        return sorted(str(f) for f in java_files)
    def find_sources(self) -> list[str]:
        """Generate compile_commands.json via Bear if missing, then return its listed sources."""
        if not Path(self.compile_commands_path).exists():
            self.run_bear()
        return super().find_sources()

When using str don't you have the risk that it isn't a valid Path? If so, shouldn't an exception be captured and rethrown in that case - to ensure a clear and correctly localized exception to the user?

I will correct that in the commit.

@FrancescoPezzella
FrancescoPezzella marked this pull request as draft September 22, 2026 12:08
@FrancescoPezzella
FrancescoPezzella marked this pull request as ready for review September 22, 2026 12:20
Comment thread test/project/test_project_scanner.py Outdated
@FrancescoPezzella

Copy link
Copy Markdown
Collaborator Author

Rebased and fixed all changes

@pjljvandelaar
pjljvandelaar merged commit 6a2758f into main Sep 23, 2026
10 checks passed
@pjljvandelaar
pjljvandelaar deleted the python-scanner branch September 23, 2026 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working infrastructure issues related to the infrastructure for building, testing, and distribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants