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

gmock_gen.py discards argument specifiers when they are defaulted #2470

Closed
wants to merge 3 commits into from

Conversation

hermas55
Copy link
Contributor

@hermas55 hermas55 commented Sep 21, 2019

Example :

class Foo {
public:
  virtual void Bar( const int a = 42 ) ;
};

Expected output :

class MockFoo : public Foo {
public:
    MOCK_METHOD1(Bar, void(const int a));
};

Actual output (const and parameter name discarded) :

class MockFoo : public Foo {
public:
    MOCK_METHOD1(Bar, void(int));
};

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@hermas55
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gennadiycivil
Copy link
Contributor

@hermas55 The scripts in the /script directory are out of date and not well supported anymore . I would be glad to merge this PR if you explain your usage.
Thank you
G

@gennadiycivil gennadiycivil self-assigned this Oct 8, 2019
@gennadiycivil
Copy link
Contributor

@hermas55 ping?

@hermas55
Copy link
Contributor Author

hermas55 commented Oct 13, 2019

Hello @gennadiycivil ,

Sure. There is as bug in the gmock_gen.py script.
It's supposed to take in input a header file of the class you want to mock, and print the mocked class in output.

It works fine as long as the class in input doesn't have methods with default parameters. When there are default parameters, the const qualifier gets discarded.

Example of Input class :

class Foo {
public:
  virtual void Bar( const int a = 42 ) ;
};

Output before the fix (const and parameter name discarded) :

class MockFoo : public Foo {
public:
    MOCK_METHOD1(Bar, void(int));
};

Output after the fix :

class MockFoo : public Foo {
public:
    MOCK_METHOD1(Bar, void(const int a));
};

The PR contains 3 commits :

  • PEP8 Automatic reformatting --> Only reformatting to comply with PEP8 standard
  • Fix for compatibility with Python 3.7 --> Make the script work with both Python 2.x and Python 3.x
  • Fix default parameters with specifiers issue --> Fix for the issue explained above + Unittests

Please let me know if you have any concerns.

Kind regards,
//HM

@gennadiycivil
Copy link
Contributor

@hermas55 Headsup, we are going to include the files under /scrips with the sync process that syncs the files internally and on GitHub. ( We will also include a disclaimer that these files are unsupported. )
All this means that there may be changes in the files you are working with. You may have to re-base this PR . we shall see

@gennadiycivil
Copy link
Contributor

@hermas55 please resolve conflicts

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@hermas55
Copy link
Contributor Author

conflicts resolved

@gennadiycivil
Copy link
Contributor

Thank you, we have started internal review. Please don't push any more changes into this PR as they might be overwritten.
(276062823)

vslashg pushed a commit that referenced this pull request Oct 28, 2019
Merge 65032e2 into 8c91ece

Closes #2470

COPYBARA_INTEGRATE_REVIEW=#2470 from hermas55:bugfix/default_const_param 65032e2
PiperOrigin-RevId: 277118535
vslashg pushed a commit that referenced this pull request Oct 29, 2019
Merge 65032e2 into 8c91ece

Closes #2470

COPYBARA_INTEGRATE_REVIEW=#2470 from hermas55:bugfix/default_const_param 65032e2
PiperOrigin-RevId: 277118535
@vslashg vslashg closed this in fff8dab Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants