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

Add !-postfix to modifying methods, prefer to extends Base methods. #232

Closed
wants to merge 2 commits into from

Conversation

twavv
Copy link
Member

@twavv twavv commented Dec 28, 2019

This should™ be fully backwards compatible.

I didn't want to add deprecation messages because that would just be cluttering.

Closes #161.

@twavv twavv requested a review from NHDaly December 28, 2019 22:54
@twavv
Copy link
Member Author

twavv commented Dec 28, 2019

CI failures related to #233.

The issue is that the size method is ambiguous with one that was deprecated between Julia 0.6 and 0.7. There are a couple of solutions.

  • Change the test to use resize! which is unambiguous (this would mean size is broken on Julia 0.7 but, again, not many people use Julia 0.7).
  • Drop support for Julia 0.7 (this requires a non-patch release).
  • Add a more specific method for size(::Window, ::Integer, ::Integer).

Any preferences? :^)

@NHDaly
Copy link
Collaborator

NHDaly commented Dec 30, 2019

Add a more specific method for size(::Window, ::Integer, ::Integer).

Yeah, i think this is the sanest thing to do. It's just keeping the existing overload that was already there. Mark it deprecatd, with a note that it's only there until we drop 0.7 support, something like:

# deprecated; delete once we drop julia 0.7 support

And then drop 0.7 in a non-patch release, later. :)

Does that sound good to you?

@NHDaly
Copy link
Collaborator

NHDaly commented Dec 30, 2019

Question: can we just drop the old methods and make a non-patch release? Do we need to keep things backwards compatible?

Or do you think that should wait for something like a 1.0.0 release? Are you preparing for that now? 😁

@twavv
Copy link
Member Author

twavv commented Dec 30, 2019

Moreso the latter. There's two things I'm thinking about.

  • We might require more API changes, in which case, we'd want to limit breaking stuff to one release (hopefully the 1.0.0 release).
  • We should release breaking changes with other improvements to increase buy-in (so increased stability, hopefully lower time-to-first-window)

src/AtomShell/window.jl Outdated Show resolved Hide resolved
src/AtomShell/window.jl Outdated Show resolved Hide resolved

# Deprecated (and misspelled?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

haha 😛 Maybe this was written by a british english speaker? Perhaps Shashi? I know he wrote a lot of this originally, right?

src/AtomShell/window.jl Outdated Show resolved Hide resolved
src/AtomShell/window.jl Outdated Show resolved Hide resolved
src/AtomShell/window.jl Outdated Show resolved Hide resolved
@NHDaly
Copy link
Collaborator

NHDaly commented Dec 30, 2019

Cool, yeah, those are good points! :) Love it.

Thanks for tackling this!

@twavv
Copy link
Member Author

twavv commented Dec 30, 2019

Think I resolved most of your comments, will give another look.

One think I'd like some input on is whether or not we should rename some methods to use underscores instead of smooshing (e.g. flash_frame! instead of flash_frame!). BlueStyle recommends underscores for non-Julia-Base code and I'm inclined to agree for consistency reasons (e.g. it's hard to find the line in the sand about where to add underscores).

The reason for this was that I was struggling to find a name for setAlwaysOnTop (this is a TODO in the PR). Options are pin! or always_on_top!. The former punts the underscore problem but 🤷‍♀️

@NHDaly
Copy link
Collaborator

NHDaly commented Dec 31, 2019

Yeah, i think adding underscores is fine. I don't really have much preference, but my opinion leans more towards underscores than not. For example, even load_file seems preferable to loadfile (although loadfile is certainly cuter 😸)

@twavv twavv closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename the setters in the Window API functions with a !
2 participants