search

antonmi / espec

Elixir Behaviour Driven Development
Other
808 stars 62 forks source link

Elixir 1.10 compatibility #297

Closed treble37 closed 3 years ago

treble37 commented 4 years ago

So just filing an issue since I realized Elixir 1.10 private API(s) which ESpec uses under the hood introduces some breaking changes which will make ESpec incompatible.

Here are the issues I found which I believe need to be addressed:

I started looking into the stale issue and found the ESpec.Diff issue as I was addressing that one. Assigning it to myself, unless someone has more time (in which case please tag me and let me know in this issue via comment) and would rather tackle any or all of the issues.

Here is a tangentially related issue brought up in the Elixir forum: https://elixirforum.com/t/psa-do-not-use-private-apis-request-a-feature-instead/15449/12

Might also be worth using this issue to figure out how to incorporate more public APIs (rather than private APIs), which is something I thought about when doing the "stale" feature but couldn't really find a good way to go about it at the time.

andrei-mihaila commented 4 years ago

Had a quick look at the diff issue - seems like a bit of work. Hopefully I can dive deeper into it in the next few days (if you haven't fixed it already).

I think your idea is very good - discussing how to incorporate more ExUnit / Elixir APIs - maybe ask the Elixir team in a ticket.

treble37 commented 4 years ago

hi @andrei-mihaila, I've mostly solved the stale issue (some warnings to clean up), but I haven't started on the diff issue, so feel free to look at it if you'd like. Let me post a request in the Elixir forum and gather some thoughts. Posted here @andrei-mihaila - https://elixirforum.com/t/incorporating-more-exunit-elixir-apis/29111 - please feel free to chime in or suggest edits to the thread if you think there are things to be asked that would elicit more clarity.

andrei-mihaila commented 4 years ago

I pushed the fixes for the diff feature to my master branch. Had to do a small fix for the stale feature so that I can run the code.

@antonmi and @treble37 - please let me know what you think (can do a pull request if all ok). @treble37 - wondering if it's easy for you to merge your fixes for the stale feature.

I think Jose Valim's idea is interesting. Will try to explore it.

treble37 commented 4 years ago

Thank you @andrei-mihaila, I like Jose's idea as well, definitely worthy of an exploration. I thought the rebase would be easy so I tested it out. The rebase against your branch using mine was pretty clean (one conflict) - here is the result - I'm ok with you making a separate PR for yours or using this combined rebase (or whatever workflow is convenient, feel free to suggest anything). My branch is functionally done, you'll see a warning around the "Some.." module which I can clean up as well if you'd like. ~The macros are~ This stuff is there as a first pass with the hope we can gradually remove the different ones that are pattern matched by version as things evolve (definitely open to suggestions from you or @antonmi as I took a fairly quick pass at this, I thought it might be better to get something out the door quickly due to the incompatibility, but perhaps this is not the case).

andrei-mihaila commented 4 years ago

My approach was similar, @treble37 (get something functional out the door quickly).

I'm ok with you doing the pull request with the rebased branch. And if you could fix the warnings that would be great. Thanks!

treble37 commented 4 years ago

Sure @andrei-mihaila - fixed and PR made.

antonmi commented 4 years ago

@treble37 @andrei-mihaila Thanks for collaboration! I've published 1.8.2 with the fix