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

WatchList Test #48

Closed
wants to merge 22 commits into from
Closed

WatchList Test #48

wants to merge 22 commits into from

Conversation

Hunsu
Copy link
Contributor

@Hunsu Hunsu commented Jan 17, 2015

I completed WatchList Test

Changed logic for WatchList query
- Removed setters from WatchResponse and replaced them with constructor
- Removed QueryParameter
- Removed pluginManager tag from pom.xml
- Changed WatchList
- I added some TODO tasks
- Added possibility to get watchlist of another user using owner
function when building Watchlist query
- Moved ParamJoiner to MediaWiki class
- Added watchlist.json ressource for WatchList testing
Completed WatchList copy method 
Completed WatchListIntegTest
added a test to WtachList
Using json format to perform a login
Use json format to get an article
added pageid to SimpleArticle

TODO:
add a tests
don't fail on checkstyle
@Hunsu
Copy link
Contributor Author

Hunsu commented Jan 24, 2015

I added Watch/Unwatch functionality. I found one bug when trying to get revisions of multiples pages (mustn't add rvlimit parameter).

@eldur
Copy link
Owner

eldur commented Jan 25, 2015

Cool thanks for you work, but as I asked before, can you give me a hint what raw change is please? You've changed about ~7000 lines with your pull request (the project has only ~20000 lines of java code)

A few questions/suggestions?

  • Why have you disabled checkstyle? Please rollback all you pom.xml changes.
  • Please reformat your changes to max length 100 and indentation with 2 spaces please, it helps me to see the real diff and speeds up the merge process
  • try to rebase and squash your commits (e.g. We don't need commits with titles like "checkstyle" or "removed print" or changes that will fail on CI Server in the history 😉 ) ... you can also force push your changes to update this pull request
  • It seems that pull Watch unwatch #49 has the same changes like WatchList Test #48, do we really need both?

@Hunsu
Copy link
Contributor Author

Hunsu commented Jan 25, 2015

Now GetRevision, GetApiToken, PostDelete, MovePage uses json format. So to be able to do this I added some Json parsing function in JsonMapper class. I added a static function in MediaWiki to check whatever a api response contains error.

I added a two functions in MediaWikiBot (watch/unwatch) to be able to a list of article to user watchlist. To do this I created a WatchUnwatchAction class. I also modified Wikibot interface to add this two function. I also added this function in Article.

Normally these are the big changes. I have tried to correct some checkstyle warnings and errors.

I have disabled checkstyle to be able to excute mvn test clean. The first time I have though that I must correct all 200 checkstyle warnings.

I have added your checkstyle file to my project configuration. Now I see checkstyle warnings in my eclipse.

Yes pull #49 and #48 are the same. I tried to make two pull request: one for the json format changes and the second for watch/unwatch functionality. I don't how to do it.

@eldur
Copy link
Owner

eldur commented Jan 25, 2015

Okay I'll try to extract these changes, but give me time, I have to check 7000 lines ... I also close pull #49 ... hmm

@eldur eldur mentioned this pull request Jan 25, 2015
@eldur
Copy link
Owner

eldur commented Jan 25, 2015

accepted in branch https://github.com/eldur/jwbf/tree/hunsu-master ... further conversion in #45

@eldur eldur closed this Jan 25, 2015
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.

None yet

2 participants