Paweł Gesek

About
  • First commit 18 May 2016
  • Latest commit 25 May 2023
Last Week All Time
Commits: 1 847

Reviews

0 active reviews

Counts may include data that you cannot access.

Line History

Line history chart not available

Line history chart not available

 

Commit Activity

Commits By Day In Week
52 week commits volume
Commits By Day In Week
Commits by day
Commits By Day In Week
Commits by hour
Commits By Hour In Day
Commit calendar
 

Committer Mappings

Paweł Gesek is mapped to 42 committers over 36 repositories
Username pgesek is mapped to:
Tuesday 13 Feb 2018
6:33 pm

Paweł Gesek

Separation of jobs might help to some extent, but the Java job will still depend on the conversion job that uses npm, since the docs are embedded as part of the Jar. The process of building a servi...

Separation of jobs might help to some extent, but the Java job will still depend on the conversion job that uses npm, since the docs are embedded as part of the Jar. The process of building a service will also get more complex.

As far as the RAML console goes, good news is that I see improvement since I last visited its repo (https://github.com/mulesoft/api-console) - they actually now have a working demo there plus some screenshots. Using this console definitely makes more sense than doing the conversion - I am well aware of the multiple limitations and additional complexity coming from it.

That being said, from what I see it is a Javascript app packaged with node. So we might run into this git issue again. Is the plan to have the RAML console packaged with services (as Swagger is currently) or as a separate standalone that's part of the distro? The latter will make the jar build process no longer deal with any of the javascript/git/npm stuff.

On a side note - how realistic is this issue with the git directory becoming incompatible? Does that happen often with git? I don't really recall many version issues between git installations.

1:44 pm

Paweł Gesek

I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my understanding a dependency that is a git repository is perfectly legal with npm - although it obviously has a lot of pitf...

I wouldn't necessarily call this a 'hack' to get around an 'issue'. From my understanding a dependency that is a git repository is perfectly legal with npm - although it obviously has a lot of pitfalls. From what I understand one of the dependencies changed to this form hence the issue.

By the way, the dev-ui image does have git installed - https://github.com/OpenLMIS/dev-ui/blob/master/Dockerfile#L8 . Wonder how many UI dependencies require it? And if it is bad to have git here, I don't see why it is ok to have it there.

If you absolutely don't want git, a workaround is to host a tarball somewhere and override the dependency with shrinkwrap or something. Getting rid of Swagger seems a bit extreme, unless this conversion to the RAML console is something we really want to invest time in.

Tuesday 28 Nov 2017
4:49 pm

Paweł Gesek

9:06 am

Paweł Gesek

I think it would be a good idea to handle a cast error here (use instanceof), seems like a more common error that can be made. It should also usually result in a 400, since from my understanding th...

I think it would be a good idea to handle a cast error here (use instanceof), seems like a more common error that can be made. It should also usually result in a 400, since from my understanding this will happen when clients ask to expand a bad field

Wednesday 15 Nov 2017
2:24 pm

Paweł Gesek

1:21 pm

Paweł Gesek

Ok, I get it, we want to treat null as a unique value

Ok, I get it, we want to treat null as a unique value

1:03 pm

Paweł Gesek

Doesn't the unique index ignore null values? Do tests fail if you remove lines 6-8?

Doesn't the unique index ignore null values? Do tests fail if you remove lines 6-8?

1:02 pm

Paweł Gesek

builder was added

builder was added

Monday 13 Nov 2017
8:47 am

Paweł Gesek

Friday 10 Nov 2017
3:53 pm

Paweł Gesek

OLMIS-3087: Rename physical_inventory_line_item_reasons
OLMIS-3087: Rename physical_inventory_line_item_reasons
Tuesday 31 Oct 2017
4:40 pm

Paweł Gesek

since these are primitives (ints) I think it's fine to use the boolean approach here, rather than boilerplate it. What do you think Josh Zamor?

since these are primitives (ints) I think it's fine to use the boolean approach here, rather than boilerplate it. What do you think Josh Zamor?

4:39 pm

Paweł Gesek

Ok looks good to me now

Ok looks good to me now

4:02 pm

Paweł Gesek

and UUID strings as values for that key.

and UUID strings as values for that key.

3:57 pm

Paweł Gesek

2:55 pm

Paweł Gesek

2:49 pm

Paweł Gesek

I think Łukasz Lewczyński is confusnig the orderid with orderlineitemid. I also don't we need this index - Paweł Albecki I suggest we drop it, unless you know of where it can be used.

I think Łukasz Lewczyński is confusnig the orderid with orderlineitemid. I also don't we need this index - Paweł Albecki I suggest we drop it, unless you know of where it can be used.

2:46 pm

Paweł Gesek

The naming of these methods is wrong: they should not start with provide, but something like 'supports'. However I believe you used 'provide' because you followed Josh's pattern, without fully impl...

The naming of these methods is wrong: they should not start with provide, but something like 'supports'. However I believe you used 'provide' because you followed Josh's pattern, without fully implementing it. From my understanding the method should be called 'provideStatusChangeExporter()' and it should return an exporter for a status change - i.e. new StatusChangeDto. If the dto does not support this field, it should return null (Josh mentioned using the new and hip Optional).

I think this is almost what we want, but not there yet, the exporters should be extended with methods like addStatusChange(StatusChangeExporter) and the provide method should be refactored to return an instance of the exporter or null (wrap it in an Optional).

Correct me if I'm wrong Josh Zamor.

2:46 pm

Paweł Gesek

Looks ok, but as I mentioned it does not fully follow the pattern. Some tweaking is needed.

Looks ok, but as I mentioned it does not fully follow the pattern. Some tweaking is needed.

2:45 pm

Paweł Gesek

Josh Zamor I've went through this review and resolved most of your comments (expect one). Can you take a look at that one and let us know if you want to be involved in resolving it. Most of your c...

Josh Zamor I've went through this review and resolved most of your comments (expect one). Can you take a look at that one and let us know if you want to be involved in resolving it.

Most of your comments are resolved through the new review that introduces the pattern you suggested, so lets continue there.

2:32 pm

Paweł Gesek

Paweł Albecki when I look at this code and javadoc, it is not immediately clear to me why one method is using a map, while the other is using a multimap. I think that's also what Josh Zamor is stru...

Paweł Albecki when I look at this code and javadoc, it is not immediately clear to me why one method is using a map, while the other is using a multimap. I think that's also what Josh Zamor is struggling here with. Can we make it more clear what is the difference between these two methods?

2:27 pm

Paweł Gesek

Josh Zamor I see you signed yourself out from the review mentioned. Can we resolve this comment? Paweł Albecki I'm not following what is happening with these different reviews - can we keep referr...

Josh Zamor I see you signed yourself out from the review mentioned. Can we resolve this comment?

Paweł Albecki I'm not following what is happening with these different reviews - can we keep referring to other reviews to a minimum?

2:25 pm

Paweł Gesek

Paweł Albecki not sure why it is in a separate review, given how small a change it is - in the future try to include such small changes in the review that has the comment. Anyway I checked the oth...

Paweł Albecki not sure why it is in a separate review, given how small a change it is - in the future try to include such small changes in the review that has the comment.

Anyway I checked the other review and the method in fact never returns null. The signature was changes as Josh Zamor suggested, resolving.

2:22 pm

Paweł Gesek

Same as with others, lets continue in the separate review. Resolving

Same as with others, lets continue in the separate review. Resolving

2:22 pm

Paweł Gesek

Marking as resolved, let's continue in the separate review

Marking as resolved, let's continue in the separate review

2:21 pm

Paweł Gesek

I verified approvedRequisitionDto has no such field as program Id. I assume it's because a batch screen only works in the context of one program. I am marking this as resolved.

I verified approvedRequisitionDto has no such field as program Id. I assume it's because a batch screen only works in the context of one program. I am marking this as resolved.

2:15 pm

Paweł Gesek

Marking as resolved, lets continue in the next review

Marking as resolved, lets continue in the next review

2:02 pm

Paweł Gesek

same comment

same comment

2:02 pm

Paweł Gesek

same comment as in previous class

same comment as in previous class

2:02 pm

Paweł Gesek

Use : "The " + getClass().getName() + " does not " be more explicit with the error.

Use : "The " + getClass().getName() + " does not "

be more explicit with the error.

1:53 pm

Paweł Gesek

Paweł Albecki I still don't get what this method does. After reading the code: can you rephrase to: "Gets a set set of UUID from the query multi value map. It looks up the map key "id" and returns...

Paweł Albecki I still don't get what this method does.

After reading the code: can you rephrase to: "Gets a set set of UUID from the query multi value map. It looks up the map key "id" and returns all values associated with that key parsed to UUIDS.