Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

Remove colored and highlight #508

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Remove colored and highlight #508

merged 4 commits into from
Jan 31, 2024

Conversation

PCSwingle
Copy link
Member

@PCSwingle PCSwingle commented Jan 31, 2024

Removes all uses of colored and highlight so that all coloring is handled entirely by the client (which is necessary for the Textual client). Ideally we eventually move completely over to styles instead of colors, but this is a step in that direction.

Pull Request Checklist

  • Documentation has been updated, or this change doesn't require that


errors = list[str]()
while self.edits:
error = self.undo()
if error:
errors.append(error)
return "\n".join(errors)
errors += error
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be .append (since it's a list)?

Copy link
Member Author

Choose a reason for hiding this comment

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

error is also a list

Comment on lines +141 to +142
if (isinstance(line, str) and line.strip()) or (
isinstance(line, Tuple) and line[0].strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is line.strip() in the if statement just checking if the line is empty or not? Might be more readable to do line == "" or line != ""

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to make sure it has stuff other than just whitespace; otherwise I would just do if line


from mentat.session_context import SESSION_CONTEXT

FormattedString = str | Tuple[str, Dict[str, Any]] | List[Tuple[str, Dict[str, Any]]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to make FormattedString an actual class instead of a union of all these types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe; I'll add a TODO but I want to get this PR merged quickly because I need it in order to work on my textual PR.

Copy link
Contributor

@waydegilliam waydegilliam left a comment

Choose a reason for hiding this comment

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

Left a few comments/suggestions, otherwise LGTM

@PCSwingle PCSwingle merged commit 32945d2 into main Jan 31, 2024
16 checks passed
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.

2 participants