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

FlxRandom: add shuffle(), deprecate shuffleArray() #1947

Merged
merged 4 commits into from
Sep 12, 2016
Merged

Conversation

ttencate
Copy link
Contributor

Deprecate FlxRandom#shuffleArray() in favour of this.

Fixes #1914.

Unlike shuffleArray(), I decided not to return the shuffled array, to make it crystal clear from the API that the array is modified in place rather than cloned.

Deprecate FlxRandom#shuffleArray() in favour of this.

Fixes HaxeFlixel#1914.
@Gama11 Gama11 added this to the 4.2.0 milestone Sep 12, 2016
@Gama11
Copy link
Member

Gama11 commented Sep 12, 2016

Wow, you even added unit tests. 👍

@ttencate
Copy link
Contributor Author

Leaving the * import from Hamcrest in place. That's the way Hamcrest was meant to be used; things get super verbose otherwise.

@Gama11
Copy link
Member

Gama11 commented Sep 12, 2016

It seems that leads to another deprecation warning though?

/home/travis/haxe/lib/hamcrest/2,0,1/org/hamcrest/collection/IsHashContaining.hx:36: characters 26-34 : Warning : Usage of this typedef is deprecated

https://travis-ci.org/HaxeFlixel/flixel/jobs/159246970#L378

@ttencate
Copy link
Contributor Author

That's in Hamcrest, here. It references the deprecated standard typedef IMap. I filed an issue against hamcrest.

@Gama11 Gama11 changed the title Add FlxRandom#shuffle() that implements a proper Fisher-Yates shuffle. FlxRandom: add shuffle(), deprecate shuffleArray() Sep 12, 2016
@Gama11
Copy link
Member

Gama11 commented Sep 12, 2016

I see... thanks.

I added an exclude to the checkstyle config, but CodeClimate doesn't seem to care much, not sure why...

The custom @:deprecated message doesn't allow concatenation it seems. Without that, it actually shows up.

@ttencate
Copy link
Contributor Author

Sorry, first contribution 😊 Thanks for cleaning up!

@Gama11
Copy link
Member

Gama11 commented Sep 12, 2016

To be fair, that @:deprecated behavior is a bit unexpected. :)

@Gama11 Gama11 merged commit accc63b into HaxeFlixel:dev Sep 12, 2016
Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
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