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

Add renaming support #536

Merged
merged 3 commits into from Nov 18, 2019
Merged

Add renaming support #536

merged 3 commits into from Nov 18, 2019

Conversation

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Nov 17, 2019

I'll add the enforced fields renaming in the same PR. Running a few tests in the meantime. :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Nov 17, 2019

Should be complete now. Just wondering about the formatting stuff...

///
/// let v = Rc::new(1);
/// let u = Rc::new(2);
/// let closure = clone!(@strong v as y, @weak u => move |x| {

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 17, 2019

Member

@GuillaumeGomez can you add test with two @Strong renamed variable too?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 17, 2019

Author Member

What would it change?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 17, 2019

Member

Just to show what syntax used in this case

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 17, 2019

Author Member

I'm not sure to follow...

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 17, 2019

Member

I was just not sure if this case work or need two @strong, now you can ignore this comment

clone!(@strong a as b, c as d => ...)

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 17, 2019

Author Member

The example you wrote doesn't work either: you have to add @strong or @weak before each variable.

This comment has been minimized.

Copy link
@sdroege

sdroege Nov 18, 2019

Member

@EPashkin anything missing here?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 18, 2019

Member

IMHO all good

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 18, 2019

Member

except for new merge conflict 😢

src/clone.rs Show resolved Hide resolved
tests/clone.rs Show resolved Hide resolved
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Nov 18, 2019

Urg. Where is this conflict coming from?! ><

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:renaming branch from a859022 to f83af02 Nov 18, 2019
@fengalin

This comment has been minimized.

Copy link
Contributor

fengalin commented Nov 18, 2019

@GuillaumeGomez: sorry, my fault, I ran ‘fmt.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Nov 18, 2019

Fixed the conflict. Let's see if the CI is still happy.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Nov 18, 2019

@fengalin fmt is definitely my personal nemesis...

fmt
@sdroege sdroege merged commit 7926c91 into gtk-rs:master Nov 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:renaming branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.