search

eldur / jwbf

Java Wiki Bot Framework is a library to maintain Wikis like Wikipedia based on MediaWiki.
http://jwbf.sourceforge.net/
Apache License 2.0
78 stars 33 forks source link

watch articles #45

Open ghost opened 9 years ago

ghost commented 9 years ago

Is it or will it be possible to watch articles (add them to personal watchlist) ?

eldur commented 9 years ago

There is currently no method in jwbf to manage your watchlist, but it could be implemented. If you want to help :wink:

Hunsu commented 9 years ago

I have started this. I succeeded to get the watchlist but I have to add some improvements.

Edit: While trying to do this I encountered many issues. Here's how I think to do this: 1- The user create an instance of WatchList by giving the bot, the limit and name spaces. 2- He can now customize its query 3- Now he can get the results from the server

My problem is when I do a third request my request contains two wlcontinue tokens. Unlike the others queries I don't create a instance of RequestBuilder each time I do a request. Nomrmally when I do: request.param(key, value1) request.param(key, value2) I get this: key=value2 not key=value1&key=value2

Second question why are you using many libraries? Why use libraries when we can do the things just by using only JDK (fro example why are you using guava and all Immutable things?)

eldur commented 9 years ago

I don't know where it is necessary to use multiple keys in mediawikis api so I decided to disable this feature. I'm not sure but I'm thinking php uses only "key=value2" so why should we send "key=value1"?

I prefer guava because it has some useful classes like the immutable ones. For more infos about immutability see javapractices or Effective Java .

And because of my preference of immutability I would create an new instance of the builder and don't allow mutation after creation (because it hard to test all permuations). :wink:

Other libs are used because I is easier for me to reuse existing utilities instead of recreating them

If you see a possibility to reduce this dependencies I'm interested in.

Hunsu commented 9 years ago

I'm willing to help, I just want to be sure about what we will do. Here's how I see how to program queries:

1- Each query class will have a constructor like this:

 public Query(MediaWiki bot, int limit, QueryParameter params, int... namespaces)

limit and namespaces are the only things that all queries share so I added to the constructor. The user have a way to parameter his query so I added a class QueryParameter in which he can parameter it.

I have programmed WatchList query with this logic (you see it in my fork, I will update it with comments). If will be ok on something I will help to redo all the queries and remove all the xml methods.

eldur commented 9 years ago

I'm checking out your fork now -- btw: it's easier to comment pull requests :wink: and it's no problem to add further commits to a pull request and/or squash/fixup commits later.

Hunsu commented 9 years ago

Ok I will do it this evening, normally I will have time.

eldur commented 9 years ago

A few comments:

    testee = WatchList.of(bot); // default watchlist
    testee = WatchList.from(bot)
      .withProperties("user", "timestamp", "comment", "title") //
      .withProperties(USER, TIMESTAMP) // or typesafer with enum values
      .onlyMinorChanges() //
      .noBotChanges() //
      .noAnonymousChanges() //
      .withLimit(1000000) //
      .build();
eldur commented 9 years ago
Hunsu commented 9 years ago

So I added the pluginManagement tag like explained here http://stackoverflow.com/questions/6352208/how-to-solve-plugin-execution-not-covered-by-lifecycle-configuration-for-sprin

eldur commented 9 years ago
  • I used setter to be able to parse the json response (if I remember well this is how Jackson library works)

You can also use constructor arguments or static factory methods

  • I have got the following error in the pom.xml: How to solve “Plugin execution not covered by lifecycle configuration” for Spring Data Maven Builds

Sounds like the eclipse pom warning, if no eclipse connector is present. In the days where I'm working with eclipse, I clicked on the error message in the pom editor and selected the option "Mark goal run as ignored in Eclipse build in Eclipse preferences"

http://stackoverflow.com/questions/13886005/how-to-undo-mark-goal-run-as-ignored-in-eclipse-build-in-eclipse-preferences

eldur commented 9 years ago

it is okay if I cherry-pick your change "Using Map instead of Multimap"?

Hunsu commented 9 years ago

Yes of course!

eldur commented 9 years ago

It's a pity, the change will cause 14 failing tests, so I have to wait

eldur commented 9 years ago

if this commit compiles and my few tweaks and TODOs are okay for you I'll merge it to master (Multimap to Map is the next change on my list) ... If it is merged, can you help with the open TODOs? (I'll recommend a hard reset)

eldur commented 9 years ago

about the topic login in a test: There is a abstract AbstractIntegTest class, check its implementaions (*IntegTest) for more details about testing mocked responses.

e.g. RecentchangeTitlesIntegTest

we need a response for a valid login and responses for watchlist queries

eldur commented 9 years ago

I'm going to push 7d2b4e9 to master this later afternoon, hope it's okay?

Hunsu commented 9 years ago

Please look at my recent commit. I don't see how to a test for WatchList. Do you have a formatter that respect your checkstyle?

eldur commented 9 years ago

I tried to add your changes to my tracking branch

I can export my formatter settings for intellij, but I'm not sure if this is useful for you. What do you think about to disable eclipses autoformatting? Or try to apply the simple rules (indent with 2 spaces, line length 100)

about testing: I try to add an example in a further commit

Please reset your branch hard to my tracking branch before you add further commits (else it becomes very difficult to resolve the conflicts) -- read the docs about git reset --hard before :wink:

btw: I've figured out how Date is mapped to String

Hunsu commented 9 years ago

Ok, just I made an error in the code. Must replace this:

if (dir != null) requestBuilder.param("wldir", dir.name()); by if (dir != null) requestBuilder.param("wldir", dir.toString());

eldur commented 9 years ago

For me it's fine to merge with master and do the next steps. Do you agree?

Hunsu commented 9 years ago

Why you want to do the date formatting in WatchList class? Why not create param method that accept Date object? These method will be used in the other queries. I suggest also to add a param method that accept long type.

eldur commented 9 years ago

It is a personal feeling maybe about Single Responsibility Principle and You aren't gonna need it (now)

Now a RequestBuilder builds a request from basic datatypes, if we add watchlist related dateformatting it will be the WatchlistDateFormattingAndAlsoUsedForOtherPurposesRequestBuilder :wink:

Personally I try to add only functionality if I need them (and a test shows me how). This means not, your idea goes the wrong direction, but until now, only Watchlist use this feature, if other queries or whatever want to use this method we'll find the way how.

btw: I don't like the Date class in public APIs and hope to update to java8 and its new date types (maybe start this task soon - java 7 End of Public Updates is scheduled for april 2015) ... then the following will be possible

.param("key", LocalDateTime.now(), ApiDateMapping::dateToString) //

.. merge? :wink:

Hunsu commented 9 years ago

Yes you can merge. Do you think that it's good idea to add a function in BaseQuery to test if the api returned an error?

BTW, I'm currently adapting the other queries to new format, do you have any comments?

eldur commented 9 years ago

Thanks

About testing: I'm working on an test example for this.

About the other queries, please reset your working branch and try to change your auto formatter :wink:

(if possible try to write tests first -- I know this could be challenging -- with the current patchset we are going to loose about 6% coverage -- goodbye green coverage badge :wink: )

eldur commented 9 years ago

I've added an test draft for Watchlist, can you help to complete it? ef7f3fd

Hunsu commented 9 years ago

Done! Can you look to the copy method and tell me how we should do to pass limit and namespaces? I created a new branch replace xml by json. Tell me if it's ok what I did for login function or do you have a better idea.

After that I will change all the queries to the format (builder + json). I think that every query must provide a limit of the results that will return;

example:

Query query = createNewQuery();
query.setLimit(500); 
// the query will return 500 results or less and then will stop
while(query.hasNext())
    doSomething(query.next());

I suggest to do that because now limit doesn't have any sense. Why the user want to change the limit?

eldur commented 9 years ago

I'm going to check your change ... thanks for your work, but give me a few minutes to restore the format (so it is easier for me to see the real change and the travis run ends with success :wink: )

btw: Have you tried to run mvn clean test locally? Do you think it is helpful if I add a contributor section to README.md with details about source code format - eg. with the title "How to configure your IDE for contributions?"

eldur commented 9 years ago

Hmm can you help me please? It feels very time consuming to check this again: Your pull request contains changes to the following files:

 src/main/assembly/src-with-dependencies.xml                                            
 src/main/java/net/sourceforge/jwbf/core/actions/HttpActionClient.java                  
 src/main/java/net/sourceforge/jwbf/core/actions/ParamJoiner.java                       
 src/main/java/net/sourceforge/jwbf/core/actions/Post.java                              
 src/main/java/net/sourceforge/jwbf/core/actions/RequestBuilder.java                    
 src/main/java/net/sourceforge/jwbf/core/internal/Checked.java                          
 src/main/assembly/src-with-dependencies.xml                                            
 src/main/java/net/sourceforge/jwbf/core/actions/HttpActionClient.java                  
 src/main/java/net/sourceforge/jwbf/core/actions/ParamJoiner.java                       
 src/main/java/net/sourceforge/jwbf/core/actions/Post.java                              
 src/main/java/net/sourceforge/jwbf/core/actions/RequestBuilder.java                    
 src/main/java/net/sourceforge/jwbf/core/internal/Checked.java                          
 src/main/assembly/src-with-dependencies.xml                                            
 src/main/java/net/sourceforge/jwbf/core/actions/HttpActionClient.java                  
 src/main/java/net/sourceforge/jwbf/core/actions/ParamJoiner.java                       
 src/main/java/net/sourceforge/jwbf/core/actions/Post.java                              
 src/main/java/net/sourceforge/jwbf/core/actions/RequestBuilder.java                    
 src/main/java/net/sourceforge/jwbf/core/internal/Checked.java                          
 src/main/java/net/sourceforge/jwbf/mapper/JsonMapper.java                              
 src/main/java/net/sourceforge/jwbf/mediawiki/MediaWiki.java                            
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/login/PostLogin.java              
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/QueryError.java           
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/Response.java             
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchList.java            
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListResults.java     
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/util/AllPagesEnums.java           
 src/main/java/net/sourceforge/jwbf/mediawiki/actions/util/LangLinksFilter.java         
 src/test/java/net/sourceforge/jwbf/core/actions/HttpActionClientTest.java              
 src/test/java/net/sourceforge/jwbf/core/actions/PostTest.java                          
 src/test/java/net/sourceforge/jwbf/core/actions/RequestBuilderTest.java                
 src/test/java/net/sourceforge/jwbf/mapper/JsonMapperTest.java                          
 src/test/java/net/sourceforge/jwbf/mediawiki/actions/editing/PostModifyContentTest.java
 src/test/java/net/sourceforge/jwbf/mediawiki/actions/queries/SearchIntegTest.java      
 src/test/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListIntegTest.java   
 src/test/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListTest.java        
 src/test/resources/mediawiki/any/embeddedin_3.xml                                      
 src/test/resources/mediawiki/v1-15/allPageTitles0.xml                                  
 src/test/resources/mediawiki/v1-15/allPageTitles1.xml                                  
 src/test/resources/mediawiki/v1-15/allPageTitles2.xml                                  
 src/test/resources/mediawiki/v1-15/siteinfo_fail.xml                                   
 src/test/resources/mediawiki/v1-16/allPageTitles0.xml                                  
 src/test/resources/mediawiki/v1-16/allPageTitles1.xml                                  
 src/test/resources/mediawiki/v1-16/allPageTitles2.xml                                  
 src/test/resources/mediawiki/v1-16/siteinfo_fail.xml                                   
 src/test/resources/mediawiki/v1-17/allPageTitles0.xml                                  
 src/test/resources/mediawiki/v1-17/allPageTitles1.xml                                  
 src/test/resources/mediawiki/v1-17/allPageTitles2.xml                                  
 src/test/resources/mediawiki/v1-17/siteinfo_fail.xml                                   
 src/test/resources/mediawiki/v1-18/allPageTitles0.xml                                  
 src/test/resources/mediawiki/v1-18/allPageTitles1.xml                                  
 src/test/resources/mediawiki/v1-18/allPageTitles2.xml                                  
 src/test/resources/mediawiki/v1-18/siteinfo_fail.xml                                   
 src/test/resources/mediawiki/v1-19/allPageTitles0.xml                                  
 src/test/resources/mediawiki/v1-19/allPageTitles1.xml                                  
 src/test/resources/mediawiki/v1-19/allPageTitles2.xml                                  
 src/test/resources/mediawiki/v1-19/siteinfo_fail.xml                                   
 src/test/resources/mediawiki/v1-20/allPageTitles0.xml

But I assume the effective change was made in the following files - can you confirm this? If not, remove or add entries from the following list, and I'll reset the rest to master.

src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/QueryError.java        
src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/Response.java          
src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchList.java         
src/main/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListResults.java  
src/main/java/net/sourceforge/jwbf/mediawiki/actions/util/AllPagesEnums.java        
src/main/java/net/sourceforge/jwbf/mediawiki/actions/util/LangLinksFilter.java      
src/test/java/net/sourceforge/jwbf/mapper/JsonMapperTest.java                       
src/test/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListIntegTest.java
src/test/java/net/sourceforge/jwbf/mediawiki/actions/queries/WatchListTest.java
eldur commented 9 years ago

ups sorry I compared the wrong branches (the post above is the diff between jwbf/master and hunsu/ChangetoJson) ... but I also interested in the effective change :wink:

Hunsu commented 9 years ago

btw: Have you tried to run mvn clean test locally? No, I will do it next time.

Do you think it is helpful if I add a contributor section to README.md with details about source code format - eg. with the title "How to configure your IDE for contributions?"

This is a good idea. Me I don't know for example how to activate loggin.

eldur commented 9 years ago

see branch https://github.com/eldur/jwbf/tree/hunsu-master

Hunsu commented 9 years ago

I'm still waiting your comments about what I did? Its strange that I haven't received a mail about your response.

eldur commented 9 years ago

sorry it is very time consuming to review your patches. It changes about ~7000 lines. (And jwbf is a free time project -- and currently my free time is very rare)

$ git diff --stat origin/master | tail -n1
 72 files changed, 3776 insertions(+), 3009 deletions(-)

If you want to help me, and speed-up the review try to reduce the diff.

Hunsu commented 9 years ago

I wish I have read this http://blog.jooq.org/2015/03/16/the-10-things-everyone-does-wrong-when-committing-pull-requests/ article before I have done the pull request. I know it hard for you to review it. The pull request is to replace xml by json. To help you review my changes you will first look at the methods I added in JsonMapper class, theses methods I use them to parse json response. If it's ok, now you will at each queries I have changed and review it. Please tell what have you have reviewed until now and make comments about I have done so I can make to something to help you review my patches.

Normally the queries are ok as the tests pass, I have just replaced the xml responses by json reponsees.

I haven't changed 7000 lines of code, There's also the resources files I have added to do the tests.

Do you any update about the "How to contribute"?

eldur commented 9 years ago

Sorry for the delay my real live job is very stressful in the last months.

To the topic of this issue: I tried to removed the format changes and squashed and reformated your change a little. See branch hunsu-master next steps will follow in this branch

eldur commented 9 years ago

If I add comments to this squashed changes, it is possible that you work with this state instead of merging back this changes?

Hunsu commented 9 years ago

Yes I can.

eldur commented 9 years ago

Hi Hunsu, the stressful days are gone and I spend some time to your patches.Your changes looks good, I've added a few comments the branch.

What do you think to rebase your changes to 3 and give them the titles ( so it's easier for me to write releasenotes later)

Hunsu commented 9 years ago

the stressful days are gone and I spend some time to your patches.

Glade to hear that, I have even started my own library.

What do you think to rebase your changes to 3 and give them the titles ( so it's easier for me to write releasenotes later)

Done. Please verify what I have done (test branch) as I'm not very familiar with git rebase. Perhaps you will have to fix check style and format the code.