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

Use 'io/console', improve JRuby and Windows support, code coverage and add 'acceptance' tests #149

Merged
merged 44 commits into from
Sep 10, 2015

Conversation

abinoam
Copy link
Collaborator

@abinoam abinoam commented Jul 20, 2015

Hi @JEG2 and all,

This PR brings the beginning of that idea of making some kind of interactive (acceptance) tests to HighLine.
Could you (all) test it with rake acceptance ?

Another thing I'm excited about is the use of 'io/console'.
Remember those comments #85 (comment) and #85 (comment) ?
I made some test here on my environment and I was able to run HighLine on JRuby using this new HighLine::Terminal::IOConsole mode.

I've put JRuby on our Travis matrix (allowed to fail) so we keep an eye on JRuby compatibility from now on.

Bumped up version to 2.0.0-develop.2

I'll let this PR open for some days to get some feedback (and perhaps add some commits more).

The complete Changelog below.

2.0.0-develop.2 / 2015-09-09

(by Abinoam P. Marques Jr. - @abinoam)

NOTES

This version brings greater compatibility with JRuby and Windows.
But we still have a lot of small issues in both platforms.
We were able to unify/converge all approaches into using io/console,
so we could delete old code that relied solely on stty, termios, java api and
windows apis (DL and Fiddle).

Another improvement is the beginning of what I called "acceptance tests".
If you type rake acceptance you'll be guided through some tests
where you have to input some thing and see if everything work as expected.
This makes easier to catch bugs that otherwise would be over-sighted.

CHANGES SUMMARY

  • Fix Simplecov - it was reporting erroneous code coverage
  • Add new tests. Improves code coverage
  • Extract HighLine::BuiltinStyles
  • Try to avoid nil checking
  • Try to avoid class variables (mis)use
  • Fix RDoc include path and some small fixes to the docs
  • Move HighLine::String to its own file
  • Add HighLine::Terminal::IOConsole
    • Add an IOConsoleCompatibility module with some stubbed
      methods for using at StringIO, File and Tempfile to help
      on tests.
    • Any enviroment that can require 'io/console' will
      use HighLine::Terminal::IOConsole by default. This kind
      of unifies most environments where HighLine runs. For
      example, we can use Terminal::IOConsole on JRuby!!!
  • Add ruby-head and JRuby (19mode and head) to Travis CI matrix. Yes, this
    our first step to a more peaceful JRuby compatibility.
  • Add AppVeyor Continuous Integration for Windows
  • Add acceptance tests for HighLine
    • Use rake acceptance to run them
    • Basically it interactively asks the user to confirm if
      some expected HighLine behavior is actually happening.
      After that it gather some environment debug information,
      so the use could send to the HighLine contributors in case
      of failure.
  • Remove old and unused files (as a result of relying on io/console)
    • JRuby
    • Windows (DL and Fiddle)
    • Termios
  • Fix some small (old and new) bugs
  • Make some more tuning for Windows compatibility
  • Make some more tuning for JRuby compatibility

ANY feedback is really appreciated!

…#ask

We shouldn't rely on setting instance variables before a method invocation.
Let's do it passing arguments only.
…Question itself

It also simplifies HighLine#ask conditionals.
At this same commit, remove the dependency on a @question instance variable.
... and fix tests accordling.

Removing unecessary _state_ handling (inst vars) is a step
toward a thiner HighLine class.

Question shouldn't be a HighLine's _state_ because the **same**
HighLine instance could be able to ask several questions.

(At the previous code @question was a kind of _current_question_ state.
But this is not really necessary currently.)
Some copyright notes above scoped class/module definitions
were mixing up with the main scope (HighLine) class documentation.
With the constricted scope ```class HighLine::String < ::String```
we would have to do a ```include HighLine::StringExtensions```

The alternative fix here was to separate the scopes like
```ruby
class HighLine
  class String < ::String
```

So we could use ```include StringExtensions```
So one could easily figure out if the "auto-detection"
is not working.
There was a "warning" being issued.
... so it doesn't shadow an argument with same name.
…output

Also save input and output as HighLine::Terminal instance variables
reducing argument passing needs.
We currently didn't find a way to test Readline on JRuby
and Rubinius without echoing to console.

At MRI Ruby we set Readline.input and Readline.output to File instances.
…O, Tempfile and File

We use StringIO, File and Tempfile at tests to mimetize STDIN and STDOUT (with io/console loaded).
This modules add some methods (stubs) to give a more complete compatibility.
These 'acceptance' tests guide the user through some steps
asking if they SEE what they should be seeing.

It summarizes the answers and adds some debug environment
information that can be sent to HighLine contributors.

It'll be a complementary way of testing HighLine without the caveats
of using a fake input/output object like StringIO as we do
on automatic unit tests.
@abinoam abinoam changed the title Refac styles Use 'io/console', improve JRuby support, code coverage and add 'acceptance' tests Jul 20, 2015
@abinoam
Copy link
Collaborator Author

abinoam commented Sep 10, 2015

Rawline is a great gem, but I stilIl have much work to do with HighLine until I have a good 2.0.0 production version to ship. I think I'm going slowly here and this is because of lack of time. So I prefer to do one thing better than burn out trying to save the world! Perhaps when things get more "calm" here I can take another gem to contribute.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 10, 2015

@djberg96 could test this gist on your setup?

https://gist.github.com/abinoam/c8e81f7c0e851bee40f4

@djberg96
Copy link

@abinoam I put a comment on the gist. Definitely something wrong. I didn't realize Luis had turned over maintenance of rb-readline to someone else. There seem to be a lot of open issues.

I also noticed that there's at least one fork of Rawline, by Zach Dennis, that seems to be getting updates. I've contacted him about taking over the project.

@JEG2
Copy link
Owner

JEG2 commented Sep 10, 2015

I just read through the commits.

This continues to be the most epically great refactoring effort I have ever seen. We must find ways to share this with the world: blog posts, conference talks, a book, or whatever. This is amazing stuff!

Almost every commit I see something else that I really love:

  • Class variables are gone
  • Examples and tests now show what mode HighLine is in
  • IO/console replaces most of the scary low-level code

Hooray!

Super minor comments:

  • Feel free to convert my horrible old habit of abusing and and or in conditionals to the correct && and ||.
  • Have you checked if control-C works when io/console is waiting on getch()? Just curious.

👍 x 1,000

Thank you so, so much!!!

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 10, 2015

😊

Thank you very much for the feedback @JEG2 !
You're so encouraging!

As I advance I'm getting more confident with the code base.
I think we could drop an e-mail at ruby-talk list and do some tweeting about this develop release.
So we could get some attention and some feedback to the next develop releases until we ship the stable 2.0.0.

When we ship the 2.0.0 we can think about spending some time to write some blog posts highlighting the most interesting things of this refactoring process.

abinoam added a commit that referenced this pull request Sep 10, 2015
Use 'io/console', improve JRuby and Windows support, code coverage and add 'acceptance' tests
@abinoam abinoam merged commit 6d8677a into JEG2:master Sep 10, 2015
@abinoam
Copy link
Collaborator Author

abinoam commented Sep 10, 2015

Merged! 👍 Closing.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 10, 2015

@djberg96 I have reported the findings in 2 issues on rb-readline

ConnorAtherton/rb-readline#123

ConnorAtherton/rb-readline#124

@djberg96
Copy link

@abinoam Thank you. And great job getting this change in!

@djberg96
Copy link

Any chance of a 2.0 prerelease gem?

@JEG2
Copy link
Owner

JEG2 commented Sep 16, 2015

This is up to @abinoam, but my opinion is that we should do that around the time we feel the changes are mostly complete.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

Hi @djberg96,

Perhaps we could just update the README to point that it is possible to run the "prerelease" version by pointing directly to the git repo at Gemfile as in:

source "https://rubygems.org"

gem "highline", git: "https://github.com/JEG2/highline.git"

What do you (@djberg96 and @JEG2) think about it?

My fear about the prerelase is that people tend to "trust" them!!! No matter how you advise them.
I think we should mature this code just a little more.
I'm not having the time that I wanted to do it, but I'm seeing some free time upcoming in 2 or 3 weeks. So I could do a "sprint" and accelerate the releases (no promises).

@JEG2
Copy link
Owner

JEG2 commented Sep 16, 2015

I prefer the README update, yes.

@djberg96
Copy link

@abinoam This is what prerelease gems are for - to get people to test drive them. If people trust them too much, that's not your problem. And, as you mentioned, they could still point at your repo with bundler, so it's not like depriving people of a prerelease gem will deter the die hards, but it does make it difficult to do standalone testing for small scripts.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

@djberg96 I may agree with the first part of what you said (that we're not saving the brave souls from using experimental code in production).

But, could you explain more detailed the part of "make it difficult to do standalone testing for small scripts"? In my superficial thinking about the problem I find it easy to add the "git" option at the Gemfile. Or just clone the repo and do a rake install on it to have the gem build and installed. Could paint the scenario more clearly so that I can be sure of it?

NOTE: I'm not a native english speaker, so sometimes is hard for me to avoid sounding rude even if I try hard to sound polite. So... if unsure, I'm really interested in your suggestion. Just need to be sure I understand it clearly.

@djberg96
Copy link

@abinoam No worries, you didn't come off as rude, and I hope I didn't either.

I suppose people could clone and install the repo, sure, so for a power user I guess there's no effective difference (except perhaps having to install whatever dev dependencies you have in the Rakefile). But, that's still more of a nuisance than just "gem install highline --prerelease". :)

But, people don't use what they don't know about. I think (and hope) that creating a prerelease gem is more likely to get more people using it, because they're more likely to spot it with a gem search or gem update. I have no hard proof, or anything, just my instincts telling me it's more likely to be seen.

Anyway, just my 0.02. It's your work, so do as you feel is best.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

Hi @djberg96 ,

I think you have convinced me.
gem install highline --pre is really compelling.

I've taken a look at http://guides.rubygems.org/patterns/#prerelease-gems

With a prerelease gem we will have (summing up):

  1. To install the stable (1-7-stable) branch:
  • gem install highline at the console
  • gem "highline" at Gemfile
  1. To install the development (master) branch:
  • gem install highline --pre at the console
  • gem "highline", ">= 2.0.0-develop.1" at Gemfile

Confirm?

So I agree with the prerelease gem. Let's see if @JEG2 sees this way too.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

I'll be disconnected for some time. Back at night. And thank you @djberg96 .

@djberg96
Copy link

@abinoam Yep, I think that's pretty much it. Maybe bundler requires a "pre" option of some sort, I don't remember right off, but I -think- what you have there works.

@JEG2
Copy link
Owner

JEG2 commented Sep 16, 2015

I'm not against the idea. I would still probably wait until we're a bit closer to done, but this is @abinoam's show and he gets to make the call.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

@djberg96 I think bundler guess the "pre" mode by checking any non-numeric character after the digits of the (semantic) version.

Most of the version specifiers, like >= 1.0, are self-explanatory. The specifier ~> has a special meaning, best shown by example. ~> 2.0.3 is identical to >= 2.0.3 and < 2.1. ~> 2.1 is identical to >= 2.1 and < 3.0. ~> 2.2.beta will match prerelease versions like 2.2.beta.12.
Source: http://bundler.io/v1.10/gemfile.html

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

but this is @abinoam's show and he gets to make the call.

magic_rabbit_icon_256

@JEG2 you made blush 😊 !!!

I have a midway solution.

As code moved from place to place some of the documentation today is completely broken.

Let me have some time to fix the documentation and then we release the "prerelease" version of the gem.

What do you think about it?

@JEG2
Copy link
Owner

JEG2 commented Sep 16, 2015

Sounds perfect to me.

@abinoam
Copy link
Collaborator Author

abinoam commented Sep 16, 2015

#154

@djberg96
Copy link

djberg96 commented Oct 6, 2015

@abinoam How's it looking?

@abinoam
Copy link
Collaborator Author

abinoam commented Oct 6, 2015

I've had some personal issues that made me late. I think Ill have time to begin by thursday.
(I've already watched some videos of the Yard creator, as I'm not good at documenting)

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.

3 participants