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

cowfile fixes #50

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

cowfile fixes #50

wants to merge 1 commit into from

Conversation

ricksbrown
Copy link
Contributor

Hi there - I am the author of the java cowsay port and have contributed to your JS port previously.

I have been testing java cowsay against native cowsay to make sure it is "accurate", not only on the standard cowfiles but also on other cowfiles I come across. This includes the roughly 190 cowfiles in your project.

My goal was to ensure the output of java cowsay matched native cowsay - you may not share the same goal, but for now I'm going to assume that you do.

I found issues in your cowfiles and initially I was fixing them as I went. In the end I changed the way my cowsay worked (it used to have an almost identical algorithm to yours) so that java cowsay would handle these in the same way as native cowsay. At this point I stopped fixing your cowfiles. This PR is what I got up to and would address most of the problems.

Specifically there are two classes of error that this applies to:

Superfluous Escapes

  1. Characters escaped superfluously, for example if a cowfile contains an escaped quote \' or hash character \# your cowsay (and my old version) will faithfully render \# whereas native cowsay will render #.

An example of this is C3PO which your cowsay will render with wonky hands and undercarriage:

 _____
< Moo >
 -----
   \
    \
       /~\
      |oo )
      _\=/_
     /     \
    //|/.\|\\
   ||  \_/  ||
   || |\ /| ||
    \# \_ _/  \#
      | | |
      | | |
      []|[]
      | | |
     /_]_[_\

But native cowsay renders it like so (note the hands are nicely aligned):

 _____
< Moo >
 -----
   \
    \
       /~\
      |oo )
      _\=/_
     /     \
    //|/.\|\\
   ||  \_/  ||
   || |\ /| ||
    # \_ _/  #
      | | |
      | | |
      []|[]
      | | |
     /_]_[_\

This problem can be addressed by fixing the cowfiles (I fixed many) or changing your code (that's what I ended up doing to java cowsay). This is not something that can be reliably solved with regular expressions. A finite state machine would be a good option and probably only take a few hours coding.

Other examples are moojira, lightbulb, cake-with-candles, biohazard and glados.

Missing Escapes

  1. The other main class of issue is where escapes have been omitted but are required for example backslash \ which will be consumed by native cowsay but rendered by JS cowsay, for example docker-whale renders with missing detail in native cowsay:
 _____
< Moo >
 -----
         \
          \
                    ##        .
              ## ## ##       ==
           ## ## ## ##      ===
       /""""""""""""""""___/ ===
  ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ /  ===- ~~~
       ______ o          __/
                     __/
          __________/

JS cowsay does not care about the missing escapes:

 _____
< Moo >
 -----
         \
          \
                    ##        .
              ## ## ##       ==
           ## ## ## ##      ===
       /""""""""""""""""\___/ ===
  ~~~ {~~ ~~~~ ~~~ ~~~~ ~~ ~ /  ===- ~~~
       \______ o          __/
         \    \        __/
          \____\______/

I decided to make java cowsay behave like native cowsay even though the output is missing detail I want it to behave as much like native cowsay as possible.

I fixed a whole lot of these but probably not all. Other examples are the yasuna cows, atat, r2d2, whale, and chito.

The other common characters missing escapes are $, for example golden-eagle and @ for example explosion

Incomplete cowfiles

Another problem is that many of your cowfiles have drifted away from being usable by native cowsay. This is most commonly due to missing a variable, usually the $eye variable, for example your kitty has no eyes in native cowsay:

 _____
< Moo >
 -----
     \
      \
       ("`-'  '-/") .___..--' ' "`-._
         `    )    `-.   (      ) .`-.__. `)
         (_Y_.) ' ._   )   `._` ;  `` -. .-'
      _.. `--'_..-_/   /--' _ .' ,4
   ( i l ),-''  ( l i),'  ( ( ! .-'    

You don't need to care, it depends how much you desire interoperability.

On that topic, your cowsay has cool yet divergent feature where $eye is replaced with a left and a right eye which may be different characters - this is not consistent with native cowsay (where $eye is a variable that will be replaced with the same value in both places).

Yasuna 08

Unparsable by native cowsay:

$ cowsay -f cows/yasuna_08.cow Moo
Scalar found where operator expected at cows/yasuna_08.cow line 7, near "$thoughts
   $thoughts"
	(Missing operator before $thoughts?)
Scalar found where operator expected at cows/yasuna_08.cow line 8, near "$thoughts
    $thoughts"
	(Missing operator before $thoughts?)
cowsay: Unrecognized character \xE2; marked by <-- HERE after        ,.:<-- HERE near column 14 at cows/yasuna_08.cow line 9.

bkendzior cowfiles

You also have these: bkendzior/cowfiles#13

@piuccio
Copy link
Owner

piuccio commented Aug 30, 2019

Amazing work! I need more time to look into this.
I see the value of having the same output as native but I don't mind to diverge, especially if we can provide bug fix and better experience.

So, for the escaping, I don't want to implement anything fancy. The original code maybe had to deal with weird characters, but nowadays I don't think it's a problem. So removing the escape symbol is my recommendation.

@ricksbrown
Copy link
Contributor Author

😄 Thanks @piuccio. I agree there is a lot to think about there and it is perfectly legit to diverge.

FYI I found another divergence just today - the original cowsay normalizes all tabs and spaces so that they become a single space:

cowsay " Moo Moo Moo "

Original cowsay:

 _______________
<  Moo Moo Moo  >
 ---------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

Cowsay JS:

 ___________________________________
<         Moo    Moo   Moo          >
 -----------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

Again, it's obviously your call (I have changed java cowsay to normalize whitespace).

@johnnysprinkles
Copy link

Good stuff, I'm referring to this on my fork of cowsay at https://github.com/johnnysprinkles/cowsay. The superfluous escapes aren't an issue there, using non-raw template literals.

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