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

Fix FunctionDef:argumentString #4085

Merged
merged 1 commit into from Oct 6, 2018

Conversation

Projects
None yet
3 participants
@muellmusik
Contributor

muellmusik commented Sep 23, 2018

Purpose and Motivation

argumentString did not handle varArgs correctly, adding a default of [ ]. varArgs cannot have defaults.

One side effect of this is that archives containing open Functions would be uninterpretable. The correct behaviour is that the open function is archived with args but does nothing. (This is what happens without varArgs. In that case it is readable.)

For example:

(
var f, g;

f = {arg ... args; g };

f.writeTextArchive("/tmp/testarch")
)

Object.readTextArchive("/tmp/testarch")

ERROR: syntax error, unexpected ‘[’
in interpreted text
line 1 char 12:

{ | args = [ ] | “open Function” }
^


ERROR: Command line parse failed

With this change the archived func is correctly rendered:

{ | ... args | "open Function" }

I think this is correct and straightforward, but it touches (lightly) core stuff, so some eyes would be good.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • All previous tests are passing
  • Tests were updated or created to address changes in this PR, and tests are passing
  • This PR is ready for review

Remaining Work

  • create tests to avoid regression (will do once we have agreement on this)
Fix FunctionDef:argumentString
```argumentString``` did not handle varArgs correctly, adding a default of ```[ ]```. varArgs cannot have defaults.

One side effect of this is that archives containing open Functions would be uninterpretable. The correct behaviour is that the open function is archived with args but does nothing. (This is what happens without varArgs.)
@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 24, 2018

Contributor

Not sure what the problems with build failures are. Restarted anyway.

Contributor

muellmusik commented Sep 24, 2018

Not sure what the problems with build failures are. Restarted anyway.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 24, 2018

Contributor

Okay seems to be happening again. Looks like at the deployment stage.

Contributor

muellmusik commented Sep 24, 2018

Okay seems to be happening again. Looks like at the deployment stage.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 24, 2018

Contributor

Appveyor seems to barf a bunch of stuff about Qt, which this branch doesn't touch (nor any compilable code).

Is there something I've done to cause this? Seems unrelated AFAICS.

Contributor

muellmusik commented Sep 24, 2018

Appveyor seems to barf a bunch of stuff about Qt, which this branch doesn't touch (nor any compilable code).

Is there something I've done to cause this? Seems unrelated AFAICS.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Sep 24, 2018

Member

@muellmusik You should get in touch with AppVeyor about it, they probably updated their build images.

Member

brianlheim commented Sep 24, 2018

@muellmusik You should get in touch with AppVeyor about it, they probably updated their build images.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 24, 2018

Contributor

@brianlheim Okay, but in the meantime, this PR can proceed, yes, as the Appveyor stuff is clearly irrelevant.

Contributor

muellmusik commented Sep 24, 2018

@brianlheim Okay, but in the meantime, this PR can proceed, yes, as the Appveyor stuff is clearly irrelevant.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Sep 25, 2018

Member

Sure, but someone should please contact them. Working CI is very important.

Member

brianlheim commented Sep 25, 2018

Sure, but someone should please contact them. Working CI is very important.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 25, 2018

Contributor

Okay, I can do that. Sorry to have to ask you @brianlheim, as I know you're sick of Appveyor stuff, but it's a bit out of my bailiwick. Should I just send them the job # and ask them if they can help?

Contributor

muellmusik commented Sep 25, 2018

Okay, I can do that. Sorry to have to ask you @brianlheim, as I know you're sick of Appveyor stuff, but it's a bit out of my bailiwick. Should I just send them the job # and ask them if they can help?

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Sep 25, 2018

Member

@muellmusik Yep, they have a support forum. https://help.appveyor.com/discussions

Thanks, I really appreciate it!

Member

brianlheim commented Sep 25, 2018

@muellmusik Yep, they have a support forum. https://help.appveyor.com/discussions

Thanks, I really appreciate it!

@muellmusik

This comment has been minimized.

Show comment
Hide comment
Contributor

muellmusik commented Sep 26, 2018

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 26, 2018

Contributor

The one failing travis build is a different issue:

Deploying application
uploading "SC-95a0e517f5e3b87942ea395528520debb284ad5a.zip" with {:content_type=>"application/zip", :acl=>"public-read"}
/Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/plugins/endpoint.rb:33:in `after_initialize': expected :endpoint to be a HTTP or HTTPS endpoint (ArgumentError)
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:84:in `block in after_initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:83:in `each'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:83:in `after_initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:21:in `initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:105:in `new'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.136/lib/aws-sdk-resources/resource.rb:169:in `extract_client'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.136/lib/aws-sdk-resources/resource.rb:15:in `initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:14:in `new'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:14:in `api'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:106:in `block (2 levels) in upload_multithreaded'
failed to deploy
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated
Contributor

muellmusik commented Sep 26, 2018

The one failing travis build is a different issue:

Deploying application
uploading "SC-95a0e517f5e3b87942ea395528520debb284ad5a.zip" with {:content_type=>"application/zip", :acl=>"public-read"}
/Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/plugins/endpoint.rb:33:in `after_initialize': expected :endpoint to be a HTTP or HTTPS endpoint (ArgumentError)
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:84:in `block in after_initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:83:in `each'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:83:in `after_initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:21:in `initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.136/lib/seahorse/client/base.rb:105:in `new'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.136/lib/aws-sdk-resources/resource.rb:169:in `extract_client'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.136/lib/aws-sdk-resources/resource.rb:15:in `initialize'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:14:in `new'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:14:in `api'
	from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.1/lib/dpl/provider/s3.rb:106:in `block (2 levels) in upload_multithreaded'
failed to deploy
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated
@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Sep 26, 2018

Contributor

Also, I don't get that the PR builds for travis and appveyor worked, but the push and branch didn't. !?

Contributor

muellmusik commented Sep 26, 2018

Also, I don't get that the PR builds for travis and appveyor worked, but the push and branch didn't. !?

@brianlheim brianlheim merged commit fb89a8b into develop Oct 6, 2018

2 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Oct 6, 2018

Member

As discussed privately CI failures are spurious.

Member

brianlheim commented Oct 6, 2018

As discussed privately CI failures are spurious.

@brianlheim brianlheim deleted the fix-FunctionDef-argumentString branch Oct 6, 2018

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Oct 6, 2018

Contributor
Contributor

muellmusik commented Oct 6, 2018

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Oct 6, 2018

Member

Oops sorry, I completely missed that! My bad.

Member

brianlheim commented Oct 6, 2018

Oops sorry, I completely missed that! My bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment