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

Adds window size and position assertions for MacOS #925

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mnmaita
Copy link
Contributor

@mnmaita mnmaita commented Oct 2, 2019

This PR tries to address Issue #700 .

Remarks:

  • I'd like to get rid of the magic numbers here and create constants for these boundaries, what would be the best way to do this?

  • I've added custom messages to the assertions, I don't know if this is desirable or not.

  • Big plus, a MacOS system user that's able to test this would be awesome 😄.

@Cobrand
Copy link
Member

Cobrand commented Oct 2, 2019

For the first one, you can have a const i32 variable inside the function WINDOW_POS_LIMIT and WINDOW_WIDTH_LIMIT or something, and then use them instead of your magic numbers.

const do not take any place in memory or in the stack, they'll be replaced as is, much like defines in C.

Otherwise it looks good to me. The alternative would be to output a Result<(), SomeKindOfError>, so that the library user can see what could go wrong at compile without his program crashing in his face.

@mnmaita
Copy link
Contributor Author

mnmaita commented Oct 11, 2019

@Cobrand Thank you for your guidelines. I want to output a Result<(), WindowSetError> (I'll need to create an enum to achieve this), but wouldn't this break the API? Right now, some of these functions already return an IntegerOrSdlError.

@Cobrand
Copy link
Member

Cobrand commented Nov 5, 2019

@mnmaita Sorry for the late response. Breaking API changes make sense if there is a valid reason behind it, and this is a perfectly valid reason.

@mnmaita
Copy link
Contributor Author

mnmaita commented Nov 14, 2019

No problem @Cobrand, I'm very busy lately but I'll get to this soon.

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