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

Added elementwise version of => to prec-pair #27447

Merged
merged 9 commits into from
Jun 9, 2018
Merged

Added elementwise version of => to prec-pair #27447

merged 9 commits into from
Jun 9, 2018

Conversation

MasonProtter
Copy link
Contributor

@KristofferC KristofferC added the needs tests Unit tests are required for this change label Jun 5, 2018
@ararslan ararslan added parser Language parsing and surface syntax needs news A NEWS entry is required for this change labels Jun 5, 2018
@JeffBezanson
Copy link
Member

Thanks @MasonProtter, and welcome!

I see you're working on porting scmutils to julia!? That's awesome. I took that class and this is something I've wanted for a while :)

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Jun 5, 2018

Thanks @MasonProtter, and welcome!

Thanks! I've been enamoured with julia for a little while now and am happy to start helping out, even if its a trivial little change here and there.

I see you're working on porting scmutils to julia!? That's awesome. I took that class and this is something I've wanted for a while :)

Yes! I haven't had any time for it in a while but I hope to get back on track soon! SICM is an awesome book and I'm hoping to learn a lot from porting scmutils. I'd like to eventually build a proper, julia native CAS with the lessons from scmutils.

@MasonProtter
Copy link
Contributor Author

Are those failed builds likely problems with the CI tools or could there actually be a problem with such a minor change?

@KristofferC
Copy link
Member

You can go into the CI logs and see what made them fail and if it is relevant. CI is not rock solid right now.

@JeffBezanson
Copy link
Member

The test case from #27446 could be added to test/broadcast.jl.

@StefanKarpinski
Copy link
Member

Looks like you've got whitespace issues.

@MasonProtter
Copy link
Contributor Author

Trying to fix now.

@@ -727,6 +727,13 @@ let f(args...) = *(args...)
@test f.(x..., f.(x..., y, z...), y, z...) == broadcast(f, x..., broadcast(f, x..., y, z...), y, z...) == 120*120
end

# Issue #27446: Broadcasting pair operator
let
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trailing whitespace is on this line

@ararslan ararslan removed the needs tests Unit tests are required for this change label Jun 8, 2018
@ararslan
Copy link
Member

ararslan commented Jun 8, 2018

It would be great if you could add a NEWS entry for this change as well.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Jun 8, 2018

What section of NEWS.md would be appropriate for this? Language changes?

@ararslan
Copy link
Member

ararslan commented Jun 8, 2018

I'd put it at the end of "new language features."

NEWS.md Outdated
@@ -14,7 +14,28 @@ New language features
* Named tuples, with the syntax `(a=1, b=2)`. These behave very similarly to tuples,
except components can also be accessed by name using dot syntax `t.a` ([#22194]).

* Keyword argument containers (`kw` in `f(; kw...)`) are now named tuples. Dictionary
* Keyw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, I think something funky happened here.

Copy link
Contributor Author

@MasonProtter MasonProtter Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, trying to fix now. Really sorry about spamming bad commits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. Though you can view the diff yourself locally before pushing with git diff origin/master. That way you can spot oddities before they show up in the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also git checkout -- NEWS.md and start over with changes to that file, which may be easier.

Copy link
Contributor Author

@MasonProtter MasonProtter Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help. It appears to be fixed now. Hard to tell from the commit difs but if you look at https://github.com/JuliaLang/julia/pull/27447/files it looks kosher.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks good now. Nice work!

@ararslan ararslan removed the needs news A NEWS entry is required for this change label Jun 8, 2018
@StefanKarpinski StefanKarpinski merged commit 2a45839 into JuliaLang:master Jun 9, 2018
@StefanKarpinski
Copy link
Member

This is such a slick little syntax that just falls out of the existing rules so nicely 💃

@MasonProtter
Copy link
Contributor Author

I was a little skeptical of the . broadcast notation initially but now I think it’s just wonderful after realizing how extensible and compossible it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants