fix error when parsing config file #820
Conversation
YAML parser returns wrong number of arguments(given 4, expected 1), this fix removes the extra parameters, while still passing in the allowAliases flag.
|
Thanks for putting this together @jschank. Do you or anyone else know what the underlying problem is? I've only been able to dedicate a few minutes to this, but haven't been able to reproduce it and don't understand why the CI passes. In the meantime, until someone figures out what the underlying problem is, I wonder if |
|
I think the problem is that the YAML lib maybe changed its signature.
It is what throws the incorrect number of parameters exception.
In the tmuxinator code, it looks like the first and last parameters are significant, the middle two were just empty arrays.
It could be that the YAML lib has a better alternative method to call. But this fix seems to work ok.
John
… On May 25, 2021, at 2:43 PM, Pete Doherty ***@***.***> wrote:
Thanks for putting this together @jschank <https://github.com/jschank>. Do you or anyone else know what the underlying problem is? I've only been able to dedicate a few minutes to this, but haven't been able to reproduce it and don't understand why the CI passes. In the meantime, until someone figures out what the underlying problem is, I wonder if YAML.method(:safe_load).arity could be used to paper over the problem?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#820 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAHM2Q3YP55EXAWTRC6MI3TPPVTRANCNFSM45CWD5DQ>.
|
|
There's been a lot of churn in the Psych library relating to this method, but I'm still not clear on what exactly changed (in libyaml or Psych) and when.
Right, but it breaks the CI build. Granted, those failures were for versions of Ruby which have been EOL'd. If that winds up being the "best" fix, then we'll need to properly drop support for those versions of Ruby. |
|
I suppose the call could be wrapped, the cause could be my attempt to use named parameters. That might have been added in a relatively recent ruby version. So old versions might be able to use one call, and newer ones, a different call.
Sadly, this gets beyond my depth with ruby though.
…Sent from my iPad
On May 25, 2021, at 5:46 PM, Pete Doherty ***@***.***> wrote:
There's been a lot of churn in the Psych library relating to this method, but I'm still not clear on what exactly changed (in libyaml or Psych) and when.
But this fix seems to work ok.
Right, but it breaks the CI build. Granted, those failures were for versions of Ruby which have been EOL'd. If that winds up being the "best" fix, then we'll need to properly drop support for those versions of Ruby.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Any thoughts on this, @tmuxinator/tmuxinator? I haven't had time to do a deep dive on the underlying issue or whether we should integrate this PR as-is and deprecate versions of Ruby which fail CI as a result or try to find a more backwards compatible approach. |
|
Hmm, my opinion is this:
1. I think we should try to be backward compatible. So I don’t think this PR is actually ready to be merged yet.
2. I think the fault (so to speak) is with the Psych library. They seem to have introduced a backward incompatible change.
3. IF there are desirable features in the new Psych release we should try to integrate with it if possible
4. IF there is no compelling reason to use the new release - Can the bundle peg the Psych release to the previous (functioning version)? Admittedly, I don’t know if ruby dependencies can be pegged like that. But if so, that would constitute the entirety of the PR, which would at least allow for more time to assess the situation.
5. IF the version of Psych can’t be pegged, then I think we accept the breaking change and bump the semver appropriately.
Anyway, that’s my thoughts, FWIW :)
… On Jun 1, 2021, at 9:07 AM, Pete Doherty ***@***.***> wrote:
Any thoughts on this, @tmuxinator/tmuxinator <https://github.com/orgs/tmuxinator/teams/tmuxinator>? I haven't had time to do a deep dive on the underlying issue or whether we should integrate this PR as-is and deprecate versions of Ruby which fail CI as a result or try to find a more backwards compatible approach.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#820 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAHM2Q2A5J243PNO6JOIIDTQTLP3ANCNFSM45CWD5DQ>.
|
|
@jschank Which version of libyaml are you seeing this problem with? If you don't happen to know, you should be able to use one of the following:
|
|
$ brew info libyaml reports version 0.2.5
one-line below seems to agree, returning 0, 2, 5
apt list won’t work - I am on a mac
… On Jun 4, 2021, at 11:35 AM, Pete Doherty ***@***.***> wrote:
@jschank <https://github.com/jschank> Which version of libyaml are you seeing this problem with?
If you don't happen to know, you should be able to use one of the following:
run this one-liner from your shell: ruby -e 'require "psych"; v=Psych.libyaml_version; puts(v.join(", "))')
apt list --installed | grep libyaml
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#820 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAHM2QVAO5GZSFQ6OYQHJDTRDXCPANCNFSM45CWD5DQ>.
|
|
Ah, okay. I thought you were on Ubuntu. For whatever it's worth, I'm not seeing the issue with libyaml 0.2.1 (Debian) or 0.2.2 (Ubuntu). Anyways, I've taken a pass at deprecating the EOL'd Ruby versions that were causing the CI build to fail. Have a look at #823 and let me know if you have any feedback. Thanks for getting the ball rolling on this fix! |
|
Looks good to me.
Glad to have been helpful :)
…Sent from my iPad
On Jun 4, 2021, at 1:28 PM, Pete Doherty ***@***.***> wrote:
Ah, okay. I thought you were on Ubuntu. For whatever it's worth, I'm not seeing the issue with libyaml 0.2.1 (Debian) or 0.2.2 (Ubuntu).
Anyways, I've taken a pass at deprecating the EOL'd Ruby versions that were causing the CI build to fail. Have a look at #823 and let me know if you have any feedback. Thanks for getting the ball rolling on this fix!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Closed by #823. |
|
@ethagnawl, do you know when there will be a new release? |
|
Great, thank you. I will test it with the next release of https://github.com/pbek/QOwnNotes 😉 |
|
I can confirm version 3.0.0 works again! Thank you very much! |
YAML parser returns wrong number of arguments(given 4, expected 1),
this fix removes the extra parameters, while still passing in the
allowAliases flag.