Skip to content

Named parameters#2

Merged
karlphillip merged 6 commits into
madplotlib:masterfrom
dtmoodie:named_parameters
Jul 2, 2017
Merged

Named parameters#2
karlphillip merged 6 commits into
madplotlib:masterfrom
dtmoodie:named_parameters

Conversation

@dtmoodie
Copy link
Copy Markdown
Contributor

Initial implementation, debugging test8. Check it out and tell me what you think.

@karlphillip
Copy link
Copy Markdown
Collaborator

I am looking at this right now, thanks!

@karlphillip
Copy link
Copy Markdown
Collaborator

I am using default MSVC 2013 and its implementation of C++11 doesn't support constexpr.
Do you think this can be solved any other way?

Also, it would be better if the parameters loose the preceding underscore, so:
plt.plot(x, y, _color = QColor(0, 0, 0));
can become:
plt.plot(x, y, color = QColor(0, 0, 0));

This is a great addition to the code, @dtmoodie

@dtmoodie
Copy link
Copy Markdown
Contributor Author

Thanks, I can look into MSVC2013. I believe most of it can be done without constexpr, it'll just require a bit of tweaking.
The reason I used '_' as a prefix is because if we globally define a variable called color, then it will alias with anyone who wants to use color in their code. What are your thoughts on putting it in a namespace then people can use a using statement to pull it into the global namespace?

@karlphillip
Copy link
Copy Markdown
Collaborator

karlphillip commented Jun 16, 2017

The reason I am inclined to push "naked" param names is to match what matplotlib offers for Python.
This will allow us to offer a much smoother transition from Python to C++ code.
If it wasn't for this, we would definitely use your suggestion because its the right way to handle these things in C++ world.

…used to place them in a separate namespace if the user doesn't want them in the global namespace.

removed the requirement for constexpr.
@dtmoodie
Copy link
Copy Markdown
Contributor Author

I think this should be a good solution, there is a define PLT_ARG_NAMESPACE which can be used to place named parameters into a namespace but by default they are globally accessible. It should also work on MSVC2013 because I compiled without constexpr on 2015. It'll be a bit until 2013 finishes downloading for me to test it there though.

@karlphillip
Copy link
Copy Markdown
Collaborator

karlphillip commented Jun 17, 2017

Ok, I'm gonna give you time to test with MSVC 2013.
Your last commit gave me:
Madplotlib.h:153: error: C3646: 'noexcept' : unknown override specifier

@karlphillip
Copy link
Copy Markdown
Collaborator

Hmm.. it compiled without problems. NICE!
OK, so I ran your code and compared the tests results to the screenshot folder:

  • Test 2 fails to draw a scatter plot correctly. It is drawing a line chart instead.

Do you think you can fix it?
Also, your final commit should not add .pro.user files to the repository. This file contains settings used by your particular setup of Qt Creator IDE.

If you are short on time I can do all that. But if I download the code and do it, you will miss a merge on Github. I can fix this by adding a Contributors.txt to the repository with your name on it.

@dtmoodie
Copy link
Copy Markdown
Contributor Author

No worries I can fix it up in the next few hours.

@karlphillip
Copy link
Copy Markdown
Collaborator

Let me know when you are done. Sometimes this page confuses me.
I see you created a new branch, nice!

@dtmoodie
Copy link
Copy Markdown
Contributor Author

Ok I think it should be good now. All the unit tests look the same as on your main page and I removed the .pro.user file.

Something to consider for is I currently have parameter inference disabled. It would be possible to have specific parameters inferred by their type. IE if you pass in just one QString and you enable inference on the marker tag, then that QString is assumed to be marker. Currently it is entirely disabled.

@karlphillip karlphillip merged commit 1924ece into madplotlib:master Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants