Skip to content

Conversation

@BenjiMilan
Copy link
Contributor

Closes #13

Finish shouldn't update the manifest only merge. Push should be what updates the manifest.

@BenjiMilan BenjiMilan requested a review from TB-1993 January 7, 2026 13:13
@BenjiMilan BenjiMilan self-assigned this Jan 7, 2026
Copilot AI review requested due to automatic review settings January 7, 2026 13:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes manifest update functionality from the finish command, ensuring that the manifest file is only updated during the push command rather than during the finish operation. This aligns with the principle that finish should only perform merges, while push handles manifest updates and remote synchronization.

Key Changes:

  • Removed _update_manifest_revisions method and its calls from the finish command
  • Cleaned up trailing whitespace and formatting inconsistencies throughout the file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Repo(self.top_dir / '.repo' / 'manifests').git.switch(base)
manifest = ScManifest.from_repo_root(self.top_dir / '.repo')
for proj in manifest.projects:
if proj.lock_status == None:
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

Inconsistent None comparison style. Line 238 uses == None while lines 220 and 229 have similar comparisons. For consistency and Python best practices, all None comparisons should use is None or is not None.

Copilot uses AI. Check for mistakes.
Repo(self.top_dir / '.repo' / 'manifests').git.switch('master')
manifest = ScManifest.from_repo_root(self.top_dir / '.repo')
for proj in manifest.projects:
if proj.lock_status == None:
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

Inconsistent None comparison style. Line 229 uses == None while lines 220 and 238 use is None. For consistency and Python best practices, all None comparisons should use is None or is not None.

Copilot uses AI. Check for mistakes.
TB-1993
TB-1993 previously approved these changes Jan 8, 2026
Copy link

@TB-1993 TB-1993 left a comment

Choose a reason for hiding this comment

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

Ship it!

@BenjiMilan BenjiMilan merged commit 274e40e into develop Jan 8, 2026
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2026
@BenjiMilan BenjiMilan deleted the feature/gh13_finish_shouldnt_update_manifest branch January 8, 2026 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: SC finish shouldn't update manifest

3 participants