Repository navigation
Added the ability to make additional parameters when creating and upd… - #330
MikhailKlimenko wants to merge 5 commits into
Conversation
|
Hi, I've separated the transport details "NameValuePair" from user-facing, added a Converter and hashset to UriConfigurator to weed out null and similar keys. |
|
I am currently traveling, will take a look at this in about a week. Thank you for helping out with this library, Mikhail! |
| import com.taskadapter.redmineapi.internal.ResultsWrapper; | ||
| import com.taskadapter.redmineapi.internal.Transport; | ||
| import com.taskadapter.redmineapi.internal.*; | ||
| import org.apache.http.NameValuePair; |
There was a problem hiding this comment.
let's avoid using wildcard imports here as well as in other places, those bring random classes at times.
| if (o == null || getClass() != o.getClass()) return false; | ||
| RequestParam that = (RequestParam) o; | ||
| return name.equals(that.name); | ||
| } |
There was a problem hiding this comment.
this may be confusing to people. why are params with the same name considered "equal"? what about the value?
if some part of the code wants to treat these Params as "equal" as long as they have equal names, it is better to keep this comparison logic close to that area, maybe with a local Comparator class or something, but not inside the data object itself. this would allow reusability for this class in the future.
| import org.apache.http.NameValuePair; | ||
| import org.apache.http.message.BasicNameValuePair; |
There was a problem hiding this comment.
these have become unused now
| // final List<NameValuePair> params = new ArrayList<>(origParams); | ||
| // if (apiAccessKey != null) { | ||
| // params.add(new BasicNameValuePair("key", apiAccessKey)); | ||
| // } |
| } | ||
| } | ||
|
|
||
| public Collection<? extends NameValuePair> convert(Collection<? extends RequestParam> origParams) { |
There was a problem hiding this comment.
this should be a private method. also, "convert" sounds very generic. something like "toNameValue" would be easier to read for people.
plus it should be static
| // if (apiAccessKey != null) { | ||
| // params.add(new BasicNameValuePair("key", apiAccessKey)); | ||
| // } | ||
| Set<RequestParam> requestParamSet = new HashSet<RequestParam>(origParams); |
There was a problem hiding this comment.
if this set creation is the main reason why RequestParam instances are considered equal if their names are equal, it is better to keep the corresponding logic right here
| .create(param1, null, param3); | ||
| assertNotNull("Checking returned result", issue); | ||
| assertNotNull("New issue must have some ID", issue.getId()); | ||
| } |
There was a problem hiding this comment.
these positive tests have a lot of duplication in them. it makes sense to extract the common parts into utility method(s)
|
I made the final changes in your branch and merged it here : #335 |
Greetings, recently I needed to develop a plugin for redmine and in the plugin I need to add additional parameters when updating and creating issue.