Conversation
…mproved CLI completion
…oyment targets for Roslyn satellites in .NET Core.
…e handling of third-party extension directories and ensure compatibility with offline paths.
- Delete redundant norm_dest assignment at line 57 that shadowed the correctly computed value from line 40 and dropped normcase - Apply normcase to existing dirs list to ensure case-insensitive comparison on Windows, preventing duplicate registrations
…rge_publish_into_bin utility for merging publish outputs. Refactor build_labs to use temporary directories for CLI and doctor builds to prevent assembly conflicts during publishing.
Small fixes before release
There was a problem hiding this comment.
PR Summary:
- Fixes build breakage caused by aggressive Renovate dependency bumps (Roslyn 5.3.0 → 4.11.0)
- Adds
System.Reflection.Metadata.MetadataUpdater.IsSupported: falseto runtime configs - Reworks
build_labsto publish CLI and doctor into temp dirs then merge, avoiding dotnet publish clobbering shared assemblies - Fixes path normalization comparison bug in Extensions manager (
normcaseapplied consistently) - Misc cleanup: unused
using System.Textmoved/removed, Go autocomplete flag reordering
Review Summary:
One functional bug was found in Extensions.smartbutton/script.py where the _ensure_path_registered function stores the normcase+normpath version of the path (lowercased on Windows) into the user config instead of the original path — this was introduced as part of making the comparison case-insensitive. The build system refactoring in _labs.py is well-motivated and the existing comment explains the ordering rationale; a minor observability improvement was noted for the merge helper. The Roslyn downgrade and the new DeployRoslynSatellitesNetCore MSBuild target are appropriate fixes for the .NET 8 build path.
Suggestions
- Add
Condition="Exists(...)"guards to theMadMilkman.Ini.dlland other unconditionalCopytasks inDeployDependenciesthat may not exist for all target frameworks. Apply - Consider adding a smoke-test step in CI that verifies both
pyrevit.exeandpyrevit-doctor.exeare present and runnable in thebin/directory after the build. Apply
⚡ Apply all quick fixes (2 quick fixes)
| normalized_existing = [os.path.normpath(d) for d in raw_dirs] | ||
| normalized_existing = [os.path.normcase(os.path.normpath(d)) for d in raw_dirs] | ||
| if norm_dest not in normalized_existing: | ||
| raw_dirs.append(norm_dest) |
There was a problem hiding this comment.
The normalized path (norm_dest) is appended to raw_dirs instead of the original dest_path. This means a normalized/lowercased path gets saved into the configuration, which may break tools that expect the originally-cased path (e.g. on Windows with mixed-case network paths, or when the stored value is displayed back to the user).
| raw_dirs.append(norm_dest) | |
| raw_dirs.append(dest_path) |
The normalization should only be used for comparison (which it already is on line 64), not for storage.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| _build("cli", configs.LABS_CLI, config=config, framework="net8.0-windows", publish_dir=cli_dir) | ||
| _build("doctor", configs.LABS_DOCTOR, config=config, framework="net8.0-windows", publish_dir=doctor_dir) | ||
| print("Merging cli and doctor publish output into bin...") | ||
| _merge_publish_into_bin(doctor_dir, configs.BINPATH) |
There was a problem hiding this comment.
The merge ordering is explained in the comment above (cli must win over doctor to preserve DocoptNet.dll), which is good. However, _merge_publish_into_bin silently overwrites shared files without any logging. If a shared DLL ends up with mismatched versions between cli and doctor outputs in the future, this will be very hard to diagnose. Consider adding a log line for overwrites in the helper:
for name in files:
dest_file = op.join(dest_dir, name)
if op.exists(dest_file):
print(f" [merge] overwriting {name}")
shutil.copy2(op.join(root, name), dest_file)actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
|
📦 New public release are available for 6.3.0.26092+2232 |
fix small bugs, mostly due to aggressive dependencies bump with Renovate (expected)