Skip to content

Fix accidental method name rename and support disabling underline #19

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

Merged
merged 3 commits into from
Feb 23, 2022

Conversation

KristofferC
Copy link
Collaborator

src/VT100.jl Outdated
@@ -167,7 +167,7 @@ end
Base.size(s::ScreenEmulator) = (s.ViewPortSize.height, s.ViewPortSize.width)
create_cell(em::ScreenEmulator,c::Char) = Cell(em.cur_cell, content = c)
cur_cell(em::ScreenEmulator) = em.cur_cell
set_cur_cell!(em::ScreenEmulator,c::Cell) = em.cur_cell = c
set_cur_cell(em::ScreenEmulator,c::Cell) = em.cur_cell = c
Copy link
Owner

Choose a reason for hiding this comment

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

It really should have a !. Can we keep both?

Copy link
Collaborator Author

@KristofferC KristofferC Feb 23, 2022

Choose a reason for hiding this comment

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

It is currently inconsistent with

set_cur_cell(em::LineEmulator,c::Cell) = em.cur_cell = c

and nothing is using the ! version right now. I can switch everything to !.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's switch everything to !. If necessary, we can keep the non-! version for compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I guess we lost CI or it would have caught this. Could you update the CI with whatever the current way we test packages is (github actions I guess?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made ! be used consistently and added CI. It is not showing up though, maybe you have to approve it first time?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it needs to be on master first for it to know to turn it on. I'll merge and see what happens.

@Keno Keno merged commit 676dd62 into Keno:master Feb 23, 2022
@KristofferC KristofferC deleted the kc/fixes branch February 23, 2022 08:15
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