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

Use disguised in final types check #762

Merged
merged 2 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@EPashkin
Copy link
Member

commented May 11, 2019

Part of #760

Removes traits from more types

Instead klass.c_class_type is klass.type_struct now used because it allow to get type from library.

cc @GuillaumeGomez, @sdroege

Some(name) => name,
};
if is_name_made_up(name) {
return None;

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 11, 2019

Member

Weird indent.

cfg_condition,
})
},
=> prepare_ctype(env, ns, t),

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 11, 2019

Member

Why going to next line?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Seems good to me. But I might have miss something so let's wait for @sdroege review. :)

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

The results look fine to me

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

IMHO we need decide about edition-2018,
then this PR and GuillaumeGomez's will need be rebased and reformated anyway.

@EPashkin EPashkin force-pushed the EPashkin:use_disguised_in_final_types_check branch from 91a2474 to 08e17db May 11, 2019

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

IMHO we should move everything to 2018. gir, its output and the manual code in the other crates.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Yes, but not right now. Let's first merge all pending PRs so we have a "clean" state to start working on.

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

Problem that #759 already added and make loot changes,
Asking to make these changes again IMHO is too much.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 11, 2019

#759 is literally just a re-run of cargo fmt (unless I'm missing something?). Therefore, it's way easier to update rather than updating the pending PRs.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

#759 is literally just a re-run of cargo fmt (unless I'm missing something?). Therefore, it's way easier to update rather than updating the pending PRs.

It also updates to 2018.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Ah. Then we have a problem because like I already said, updating pending PRs would be a nightmare. So we have the current choice:

  • Update to 2018 and rewriting the pending PRs (let's be honest, rebasing won't work)
  • Merge the pending PRs and rebase/rework this work.

I obviously prefer the second option but what do you think @EPashkin, @sdroege?

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

I for first.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Either way seems fine to me. It shouldn't be too hard to update the 2018 PR. One part is "cargo fmt", the other is manually updating everything for 2018. The second part should be easy to merge, the first part can be automatically redone.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

We should just get the 2018 PR in really soon. Let's put tomorrow early afternoon as deadline, every other PR that is not ready before then will have to be rebased on top of that.

@EPashkin EPashkin force-pushed the EPashkin:use_disguised_in_final_types_check branch from 08e17db to 1318a1d May 13, 2019

@EPashkin EPashkin merged commit 2daab19 into gtk-rs:master May 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:use_disguised_in_final_types_check branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.