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 missing dependency for Random, clean up duplicate lib in test.hxml #2

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
2 participants
@bsinky
Collaborator

bsinky commented Oct 9, 2017

340f006 introduced a dependency on polygonal-core.

Also noticed that hxslam is referenced twice in test.hxml and had to clean it up.

Slightly off-topic, but I also am unable to find references to hamcrest inside the code, besides the -lib references in various .hxml files. Are there future plans to implement features that depend on hamcrest or am I missing something?

Lastly, what's your opinion on dropping the required dependency on hxslam? It's required in the haxelib.json file, but is only truly required to run the unit test suite. None of the actual .NET API implementations depend on the short lambda feature. Technically, an end user could install and run haxesharp without installing hxslam. The choice of whether their project should depend on hxslam could be left to them entirely.

I can understand why you might require hxslam anyway, in that it does add lambdas that are more familiar to .NET developers to Haxe, but from a technical standpoint it doesn't seem necessary for haxesharp.

@nightblade9

This comment has been minimized.

Owner

nightblade9 commented Oct 10, 2017

  • Thanks, I forgot about the haxelib change for polygonal-core.
  • hamcrest is a "copy pasta" (copy-paste) mistake from wherever I copied the test.hxml file from (probably lime). You can delete that in your PR (or I can delete it later).

@nightblade9 nightblade9 reopened this Oct 10, 2017

@nightblade9 nightblade9 merged commit 893de7c into nightblade9:master Oct 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nightblade9

This comment has been minimized.

Owner

nightblade9 commented Oct 10, 2017

About hxslam, my opinion is that it should probably stay until Haxe 4.0 is out (which includes short lambdas). I would like haxesharp to be "batteries included," meaning it includes short lambdas out of the box; adding it as a dependency means you just include haxesharp and you can write LINQ-like queries. (Those unit tests are supposed to be somewhat representative of the kind of code users would write.)

I assume most haxesharp consumers are not particularly Haxe-savvy: meaning, they don't want to deal with the idiosyncrasies of Haxe in places where C# provides simpler APIs. The goal is to make Haxe more approachable and usable without having a deep knowledge of or experience with the core language.

That said, providing an option to switch the short-lambdas library out at some point might have made sense; except that Haxe 4.0 ships with it, so it's going way shortly.

Does that make sense?

@bsinky

This comment has been minimized.

Collaborator

bsinky commented Oct 10, 2017

Ok, I didn't know Haxe 4.0 was adding short-lambdas to the language. 👍 That makes sense then.

@bsinky bsinky deleted the bsinky:missing-polygonal-core-dependency branch Oct 10, 2017

@nightblade9

This comment has been minimized.

Owner

nightblade9 commented Oct 10, 2017

I would normally be more hesitant about Haxe releases (which are few and far between) but I noticed two things:

This makes me cautiously optimistic that we can do away with hxslam in the next year or so, and don't have to worry about potentially supporting multiple short-lambda libs 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment