Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix YAML Regression in 1.9 #16

Closed
wants to merge 2 commits into from

3 participants

@5long
Collaborator

After I could build a sup.gem locally, the first program I run was sup-config. Syck and Psych behave differently and current codebase just assumes Syck is used everywhere. Problem is, sup-config loads yaml(in 1.9 that's Psych) before lib/sup.rb, and the whole ruby process just uses Psych all the way down, producing this bug.

This patch fixed it.

5long added some commits
@5long 5long A regression test for YAMLing in 1.9 5d31e5e
@5long 5long Ensure Syck is used.
In the long run we'd better drop Syck in favor of Psych
a3d313f
@5long
Collaborator

I've rebased this branch against upstream/develop so it builds on Travis. 15.1 builds successfully and 15.2 failed due to date format of ncursesw gem.

@gauteh
Owner

The fail against the date format of ncursesw: is this a real issue with all ruby 1.8 installs?

@5long
Collaborator

I believe it's an issue of Ruby 1.8 and a recent version of rubygem. I'm thinking about switching to ncurses-ruby which does support wide char display on my Arch box. But I'm not sure if it works on Mac and Ubuntu (and other linux distros).

@gauteh
Owner

As mentioned in #17, Ruby 2.0 seems to have dropped syck altogether. I think now is the time to go Psych.. since ensuring 'syck' won't work in Ruby 2.0.

@5long
Collaborator

At first I'm working toward a Ruby 1.8-compatible release instead of 2.0-compatible one. But dropping 1.8 support would make it a lot easier to release a usable version. I'm glad to see this pull request closed if that matches the roadmap.

@gauteh
Owner

Yes, as much as I would like a usable version right away I would prefer to do it right. A possibility is to just branch of a quick-and-dirty 1.8/1.9 bugfix release (which doesn't need any migration)? I think this patch would go against finally migrating to Psych.

A 1.8/1.9 bugfix release could also go into the roadmap for preliminary step. What do you think?

@gauteh
Owner

By the way; I don't care much for 1.8 or 1.9 - I am all for only 2.0.0 if that is an option.

@5long
Collaborator

Personally I'm using 1.9 for all my day-to-day Ruby apps(Jekyll, Vagrant) so I'm okay with a 1.9/2.0 only release.

I'm more toward a quick-and-dirty approach so I can just start using it without too much hassle. Besides, I just hesitate to do large-scale migration with enough familiarity with the existing code base. Especially when I'm not certain how much time I might put into sup's development.

So my advice is: find out who's willing to put the most effort into sup development, and let him/her decide :)

@gauteh
Owner

Well said. I neither have much time to spare.

Cheers, Gaute

@gauteh
Owner

Did some testing on this branch, and it seems to work fine on 1.9.3. I think getting ruby 2, psych and all that working requires some 1.9.3/syck breakage anyway. If noone objects I'd merge this one.

@ericweikl
Owner

+1 Just gave it a try and didn't notice any problems. Kudos for adding a test :-)

@gauteh gauteh referenced this pull request from a commit
@5long 5long Merge #16: Fix YAML regression in 1.9
Squashed commit of the following:

commit a26b99c64e2da4115ea47606ae8b66f0b008b002
Author: Whyme Lyu <callme5long@gmail.com>
Date:   Tue Mar 26 23:03:50 2013 +0800

    Ensure Syck is used.

    In the long run we'd better drop Syck in favor of Psych

commit a99a2b244063c2088d91688f667ee68f05f24df0
Author: Whyme Lyu <callme5long@gmail.com>
Date:   Tue Mar 26 23:03:25 2013 +0800

    A regression test for YAMLing in 1.9
ac82b28
@gauteh gauteh referenced this pull request from a commit in gauteh/sup
@gauteh gauteh Revert "Merge #16: Fix YAML regression in 1.9"
This reverts commit ac82b28.
b93d1e3
@gauteh gauteh referenced this pull request from a commit in gauteh/sup
@gauteh gauteh Get the relevant stuff from issue #16.
From ac82b28.
d380c03
@gauteh gauteh closed this
@foobacca foobacca referenced this pull request from a commit in foobacca/sup
@foobacca foobacca Merge remote-tracking branch 'origin/develop' into develop
* origin/develop: (21 commits)
  set email on gem to sup-talk
  Explicitly require 'rbconfig' before using it
  s/Config/RbConfig for less warnings
  Allow builds on 2.0 fail for now
  Merge and close #24: Remove old and abandoned Ditz bug system and bugs
  sup-config: call to_s() on user input
  Merge #16: Fix YAML regression in 1.9
  Added simple auto-completion to search prompt
  Fix problem with account selector
  upd webpage to supmua.org
  Update name and adresses to our fork
  Fix updating contact aliases. Fixes an UndefinedMethodError. I lost one too many emails.
  add ruby 2.0.0 to rvm
  Merge #15: Integrate Travis
  Fix #12: Require 'iconv' before patching it
  Make the tests pass again
  Fix #10: Use a maintained fork of xapian-full
  Fix #9: Convert use of Rake::GemPackageTask to Gem::PackageTask
  Fix #5: Require relative that works on both ruby 1.8 and 1.9
  Added new hook 'gpg-expand-keys'
  ...
5a3b194
@5long 5long referenced this pull request
Merged

Don't set yamler for 1.8 #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 30, 2013
  1. @5long
  2. @5long

    Ensure Syck is used.

    5long authored
    In the long run we'd better drop Syck in favor of Psych
This page is out of date. Refresh to see the latest.
Showing with 20 additions and 1 deletion.
  1. +0 −1  bin/sup-config
  2. +3 −0  lib/sup.rb
  3. +17 −0 test/test_yaml_regressions.rb
View
1  bin/sup-config
@@ -2,7 +2,6 @@
require 'rubygems'
require 'highline/import'
-require 'yaml'
require 'trollop'
require "sup"
View
3  lib/sup.rb
@@ -1,6 +1,9 @@
require 'rubygems'
+
require 'syck'
require 'yaml'
+YAML::ENGINE.yamler = 'syck'
+
require 'zlib'
require 'thread'
require 'fileutils'
View
17 test/test_yaml_regressions.rb
@@ -0,0 +1,17 @@
+require 'test/unit'
+
+# Requiring 'yaml' before 'sup' in 1.9.x would get Psych loaded first
+# and becoming the default yamler.
+require 'yaml'
+require 'sup'
+
+module Redwood
+ class TestYamlRegressions < Test::Unit::TestCase
+ def test_yamling_hash
+ hsh = {:foo => 42}
+ reloaded = YAML.load(hsh.to_yaml)
+
+ assert_equal reloaded, hsh
+ end
+ end
+end
Something went wrong with that request. Please try again.