Skip to content

BIM: drop SH3D importer docstring args the methods do not take - #31773

Merged
Roy-043 merged 1 commit into
FreeCAD:mainfrom
darkdi:fix/bim-sh3d-docstring-args
Aug 6, 2026
Merged

BIM: drop SH3D importer docstring args the methods do not take#31773
Roy-043 merged 1 commit into
FreeCAD:mainfrom
darkdi:fix/bim-sh3d-docstring-args

Conversation

@darkdi

@darkdi darkdi commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Six Args: entries in importers/importSH3DHelper.py name a parameter the method does not take. Docstrings only — nothing else in the file changes.

  • SH3DImporter.get_floor and BaseHandler.get_floor both document levels; both take only level_id.
  • SH3DImporter._import_elements documents update_progress, including a "Defaults to True" that has nothing to default.
  • LevelHandler._extrude documents floor; it takes only obj_to_extrude.
  • WallHandler._create_facebinders and ._create_baseboards both document elm, the XML element. Neither receives it — they work from wall and floor.

Each one was opened and read against its signature.

Two more of the same kind sit in CAM (Path/Tool/shape/models/icon.py and Path/Tool/toolbit/util.py). I left them out to keep this to one workbench; happy to send them separately.

@github-actions github-actions Bot added Status: Needs triage Missing triage Mod: BIM Related to the BIM Workbench Language: Python labels Aug 6, 2026
@Lgt2x

Lgt2x commented Aug 6, 2026

Copy link
Copy Markdown
Member

did you use a script to find those?

@darkdi

darkdi commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Yes — a small script that parses each Args: block and compares the names against the signature above it, nothing cleverer than that.

It is wrong often enough that I don't trust it: on other projects roughly a third of what it reports turns out to be legitimate. A name can belong to a @property, or to a callback whose arguments the docstring is describing, or to a *args passthrough where the entry is the only hint the reader gets. So every finding gets opened and read against the code before I touch it, and here I dropped the two in CAM rather than send a second workbench in the same PR.

Happy to share the script if it would be useful to you.

@maxwxyz maxwxyz added Type: Other Tasks, refactoring, quality improvement, actions, dependencies, packaging, translations or other... Requires: Code Review The PR requires code to be reviewed and removed Status: Needs triage Missing triage labels Aug 6, 2026
@maxwxyz maxwxyz moved this from Queue to Merge Meeting in Merge Queue Aug 6, 2026
@maxwxyz maxwxyz added this to the 26.3 milestone Aug 6, 2026
@Roy-043 Roy-043 added Approved: Code Quality The PR was checked for code quality and approved and removed Requires: Code Review The PR requires code to be reviewed labels Aug 6, 2026
@Roy-043

Roy-043 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Thanks.

Two more of the same kind sit in CAM ... happy to send them separately.

Please do.

@Roy-043
Roy-043 enabled auto-merge (squash) August 6, 2026 13:03
@Roy-043
Roy-043 merged commit d2501ef into FreeCAD:main Aug 6, 2026
33 of 34 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Completed in BIM development Aug 6, 2026
@github-project-automation github-project-automation Bot moved this from Merge Meeting to Done in Merge Queue Aug 6, 2026
rodgomesc pushed a commit to rodgomesc/FreeCAD that referenced this pull request Oct 18, 2026
@darkdi @rodgomesc
BIM: drop SH3D importer docstring args the methods do not take (FreeC…
fcbf8b7 
…AD#31773)

Co-authored-by: darkdi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Approved: Code Quality The PR was checked for code quality and approved Language: Python Mod: BIM Related to the BIM Workbench Type: Other Tasks, refactoring, quality improvement, actions, dependencies, packaging, translations or other...

Projects

BIM development
Status: Completed
Merge Queue
Archived in project

Milestone

26.3

Development

Successfully merging this pull request may close these issues.

4 participants

@darkdi @Lgt2x @Roy-043 @maxwxyz