search

irods / irods_client_http_java

test wrapper for iRODS HTTP API
0 stars 1 forks source link

Adding Tests for query endpoint #51

Closed sam-i-am012 closed 3 months ago

sam-i-am012 commented 3 months ago

Addresses #21 Also added logging using Log4j 2 (#46)

korydraughn commented 3 months ago

Moving forward, let's try to keep the number of issues addressed in a PR down to one. That makes reviewing the PR much easier/faster.

Focus on resolving a single issue. If you can address multiple issues without them interleaving, that's where opening multiple PRs makes sense.

What's the status of this PR?

sam-i-am012 commented 3 months ago

Oh sorry, I was only adding it to the query endpoint, not to all the other ones. I just figured it might be convenient to begin adding loggers here and then add them to the rest of the tests as I go along instead of having to go through every test again at the end to add the logging. I believe this PR is ready for review, I added an extra commit last night when I realized I forgot to make a variable private.

korydraughn commented 3 months ago

No problem. Please squash things to taste and/or attach issue numbers to the commits without them and then I'll do a pass over the code.

korydraughn commented 3 months ago

We don't receive notifications for force-pushes. Anytime you do that, post a comment so we're notified.

sam-i-am012 commented 3 months ago

I've just finished squashing everything.

sam-i-am012 commented 3 months ago

All existing comments have been resolved. Awaiting further review.

sam-i-am012 commented 3 months ago

All existing comments have been resolved. Awaiting further review.

sam-i-am012 commented 3 months ago

All comments have been resolved.

sam-i-am012 commented 3 months ago

I removed some variables that I realized I wasn't using.

korydraughn commented 3 months ago

Please run the tests and if they pass, squash the commits to taste.

sam-i-am012 commented 3 months ago

Done.

sam-i-am012 commented 3 months ago

I have resolved the comments.

sam-i-am012 commented 3 months ago

Yes, tests are all passing and if I remove the LevelRangeFilter from only displaying logs at the error level, the messages are written to console.

korydraughn commented 3 months ago

I'm not familiar with the level range filter. What does that do?

Normally, the person just changes the log level to get more output (e.g. from info to debug or trace).

sam-i-am012 commented 3 months ago

It should only write the message to the console when its on the error level, but I got that from watching a youtube tutorial and that's what he did. I could just remove it too if that is not the norm.

korydraughn commented 3 months ago

No, it's fine as is.

So for now, if the tests are passing, squash to taste.

korydraughn commented 3 months ago

Are you happy with the commits? Has everything been squashed to your liking?

sam-i-am012 commented 3 months ago

Sorry, didn't see this comment. I'm squashing now.

sam-i-am012 commented 3 months ago

Done.

sam-i-am012 commented 3 months ago

Done.

korydraughn commented 3 months ago

Merging.