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 js and py files for API pageswithprop #185

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

PRISCILAVILEMEN
Copy link
Contributor

Created files that make requests to this API and printed the page titles as examples.

@PRISCILAVILEMEN
Copy link
Contributor Author

Hi @brendajerop,
can you help me reviewing my first pr? :)

Copy link
Contributor

@brendajerop brendajerop left a comment

Choose a reason for hiding this comment

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

@PRISCILAVILEMEN Thank you for working on this!
Some general comments:

  • We need the PHP code as well, so please add it.
  • In your commit, make sure you add API:Pageswithprop to the readme files of Python, PHP and JavaScript folders.
  • Please provide a link to your sandbox page for this module.
  • We don't need to modify .gitignore.

@PRISCILAVILEMEN
Copy link
Contributor Author

PRISCILAVILEMEN commented Oct 16, 2019

@PRISCILAVILEMEN Thank you for working on this!
Some general comments:

  • We need the PHP code as well, so please add it.
  • In your commit, make sure you add API:Pageswithprop to the readme files of Python, PHP and JavaScript folders.
  • Please provide a link to your sandbox page for this module.
  • We don't need to modify .gitignore.

@brendajerop , Thaks for the return..

I want to know
1 * We need the PHP code as well, so please add it.

I need to install something to run the code?

2- * Please provide a link to your sandbox page for this module.
https://www.mediawiki.org/wiki/User:Prisicilavilemen/Sandbox/API:Pageswithprop

Is to Add this link?

@brendajerop
Copy link
Contributor

@brendajerop , Thaks for the return..

I want to know
1 * We need the PHP code as well, so please add it.

I need to install something to run the code?

Which operating system do you use?

2- * In your commit, make sure you add API:Pageswithprop to the readme files of Python, PHP and JavaScript folders.

about this task, I add the API, but the when click in the link, had error

I see that you have already added the files :)

3- * Please provide a link to your sandbox page for this module.
https://www.mediawiki.org/wiki/User:Prisicilavilemen/Sandbox/API:Pageswithprop

Is to Add this link?

Yes this is the link:)

@PRISCILAVILEMEN
Copy link
Contributor Author

PRISCILAVILEMEN commented Oct 25, 2019

@brendajerop , I need to know where I input this link : https://www.mediawiki.org/wiki/User:Prisicilavilemen/Sandbox/API:Pageswithprop , Can you help me please ;)

@brendajerop
Copy link
Contributor

@brendajerop , I need to know where I input this link : https://www.mediawiki.org/wiki/User:Prisicilavilemen/Sandbox/API:Pageswithprop , Can you help me please ;)

In the pull request description or the comment section. You've already linked to it so that's fine :)

Copy link
Contributor

@brendajerop brendajerop left a comment

Choose a reason for hiding this comment

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

@PRISCILAVILEMEN Thank you for working on this!
Since API:Pageswithprop is a GET request, the code should be auto-generated. How you do that is by adding its JSON schema to modules.json, then run python autogenerator.py to autogenerate the code. Please see the last statement in the contributing guidelines. However, I don't see the modules.json file in your pull reqest or the mediawikijs code. The pull request should contain changes to nine files:

  • modules.json
  • javascript/README.md
  • python/README.md
  • php/README.md
  • mediawikijs/README.md
  • python/get_pageswithprop.js(autogenerated)
  • mediawikijs/get_pageswithprop.js(autogenerated)
  • php/get_pageswithprop.js(autogenerated)
  • javascript/get_pageswithprop.js(autogenerated)

@PRISCILAVILEMEN
Copy link
Contributor Author

@brendajerop hi,
I read this info at contributing.md (located on the root directory)
It says:

"For GET requests modules only, python, javascript and php sample code can be auto-generated by following the instructions below:"

As this file says can be auto-generated not should be auto-generated, so should I update this file to avoid further misleading interpretations?

@PRISCILAVILEMEN
Copy link
Contributor Author

I made the changes, please check, and give me feedback, thanks


$output = curl_exec( $ch );

if (curl_errno($ch)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this if statement do? Do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This " if statement" existed to identify errors in curl and I forgot to remove it while fixing the SSL certificate issue.
take advantage and ask if they use any code editor or develop directly on the platform?

$ch = curl_init($url);

curl_setopt( $ch, CURLOPT_RETURNTRANSFER, true );
//curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
Copy link
Contributor

@brendajerop brendajerop Oct 29, 2019

Choose a reason for hiding this comment

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

Remove this comment:)

@PRISCILAVILEMEN
Copy link
Contributor Author

@brendajerop , review my code ;) and give more feed backs

Best Regards

@brendajerop
Copy link
Contributor

brendajerop commented Oct 30, 2019

@PRISCILAVILEMEN Please rebase your changes on top of those in master. To do this, run git pull --rebase upstream master:)

Copy link
Contributor Author

@PRISCILAVILEMEN PRISCILAVILEMEN left a comment

Choose a reason for hiding this comment

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

@PRISCILAVILEMEN Please rebase your changes on top of those in master. To do this, run git pull --rebase upstream master:)

Hello @brendajerop , I rebased my repo and fixed the merge conflicts. Could you verify , if everything is ok ?


$output = curl_exec( $ch );

if (curl_errno($ch)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This " if statement" existed to identify errors in curl and I forgot to remove it while fixing the SSL certificate issue.
take advantage and ask if they use any code editor or develop directly on the platform?

@brendajerop
Copy link
Contributor

@PRISCILAVILEMEN What do you get when you run git pull --rebase upstream master?

@PRISCILAVILEMEN
Copy link
Contributor Author

@brendajerop , Sorry my rebase was not complete, but now I updated my branch and merged, please check. thanks

@PRISCILAVILEMEN
Copy link
Contributor Author

PRISCILAVILEMEN commented Oct 31, 2019

@PRISCILAVILEMEN What do you get when you run `git pull --rebase upstream master?

My repo was a lot of commits behind, here is what I did before:

  • Rebased origin master from upstream master
  • Rebased origin mw-pageswithprop from origin master

But it seems that all the changes weren't there.

I did rebase again on my branch directly from upstream master, and it seems that now it merged correctly.

This project is my first experience with git, but I think I am getting used to it.

Can you please give another look at the PR? 😄

@brendajerop brendajerop merged commit fd8fbe1 into wikimedia:master Nov 1, 2019
@PRISCILAVILEMEN PRISCILAVILEMEN deleted the mw-pageswithprop branch November 1, 2019 09:34
@brendajerop
Copy link
Contributor

@PRISCILAVILEMEN I've left some feedback on the discussion page of your sandbox: https://www.mediawiki.org/wiki/User_talk:Prisicilavilemen/Sandbox/API:Pageswithprop :)

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