Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improve operation messages and make them stateful #9245

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bswck
Copy link
Member

@bswck bswck commented Mar 27, 2024

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

This PR proposes more meaningful messages on installation operations:

  • "Writing lock file" was displayed after writing the lock file. Now the message is "Lock file written", but I'm happy to adapt to a different idea that introduces past tense.
    The key is to convey the true state of the lock file; looking at
    updated_lock = self._locker.set_lock_data(self._package, repo.packages)
    if updated_lock:
    self._io.write_line("")
    self._io.write_line("<info>Writing lock file</>")

    one could easily observe that the "Writing lock file" message is written if updated_lock is truthy—and can only be after actually completing writing the lock file:
    def set_lock_data(self, root: Package, packages: list[Package]) -> bool:
    """Store lock data and eventually persist to the lock file"""
    lock = self._compute_lock_data(root, packages)
    if self._should_write(lock):
    self._write_lock_data(lock)
    return True
    return False

    def _write_lock_data(self, data: TOMLDocument) -> None:
    lockfile = TOMLFile(self.lock)
    lockfile.write(data)
    self._lock_data = None

    Where TOMLFile.write() atomically writes to the file.
  • In terminals supporting ANSI output, the "Installing the current project" message shifts to "Installed the current project" when finished.
  • Operation objects were partially stateful (e.g. held the skipped attribute) and now are fully stateful (store the information about whether they are in the done, warning or error state). Different attribute names ideas are welcome.
  • Executor.get_operation_message() was deprecated and polymorphism was utilized in lieu of it. The new implementation was made to be 100% equivalent except that after the relevant operations finish, their messages use past tense in ANSI supporting cases–instead of freezing at "Downgrading" if Update(...).done is True, "Downgraded" is written eventually, etc.
  • Instead of "Removing... (...): Removing..." and similar messages for other job types, one will now read "Removing ... (...): In progress..." as long as the operation is in progress. Other ideas welcome.

Before this can eventually be merged, we need a new GIF in the Poetry readme. @neersighted could you help with that?

@bswck bswck changed the title feat: make operation messages and make them stateful feat: improve operation messages and make them stateful Mar 27, 2024
@bswck
Copy link
Member Author

bswck commented Mar 27, 2024

I recorded "before and after" with asciinema for poetry add pydantic and poetry remove pydantic commands in the poetry repo.
What concerns me is that the operation messages starting with hyphens no longer become MarkDown bullet points.
(Ignore the a command and the sync-pre-commit-lock extension).

Before After
before after

@Secrus Secrus requested a review from a team March 27, 2024 23:40
@@ -351,7 +351,7 @@ def _write_lock_file(self, repo: LockfileRepository, force: bool = False) -> Non

if updated_lock:
self._io.write_line("")
self._io.write_line("<info>Writing lock file</>")
self._io.write_line("<info>Lock file written</>")
Copy link
Member

Choose a reason for hiding this comment

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

Could we have this stateful as well? Like, Writing lock file changing to Lock file written once the operation is done?

Copy link
Member Author

@bswck bswck Mar 28, 2024

Choose a reason for hiding this comment

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

Well then, I guess that a new IO section would be needed and the

if self._should_write(lock):
self._write_lock_data(lock)
return True

snippet would probably have to be rewritten to

     if self._should_write(lock): 
         section = ...
         section.write_line("<info>Writing lock file</>")
         self._write_lock_data(lock) 
         section.write_line("<info>Lock file written</>")  # or <success>Lock file written</>
         return True 

Is it worth the cost? It would require introducing a side effect in _write_lock_file or adding some extra logic to set_lock_data. IO writes are quite fast in contrary to installation operations; the only advantage it would bring is that it could inform about a potentially (but very rarely) erroneous action taken (in case the write fails) useful as a context for a traceback following the message.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then maybe we could do something like Writing lock file... Done. where Done. appears after the operation is finished?

Copy link
Member Author

@bswck bswck Mar 28, 2024

Choose a reason for hiding this comment

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

Ok, then maybe we could do something like Writing lock file... Done. where Done. appears after the operation is finished?

I think that would be inconsistent with the rest of the UI. Also, would it address the issue with additional logic/necessity of introducing new _write_lock_file side effects somehow?

@bswck bswck force-pushed the feat/stateful-operations branch 2 times, most recently from bf89285 to e80c1e8 Compare August 22, 2024 15:26
@bswck bswck force-pushed the feat/stateful-operations branch from e80c1e8 to c3648d9 Compare August 22, 2024 15:29
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