Skip to content

RESTWS-813: Compared strings using .equals instead of == and !=#448

Closed
insookwa wants to merge 7 commits intoopenmrs:masterfrom
insookwa:RESTWS-813
Closed

RESTWS-813: Compared strings using .equals instead of == and !=#448
insookwa wants to merge 7 commits intoopenmrs:masterfrom
insookwa:RESTWS-813

Conversation

@insookwa
Copy link

@insookwa insookwa commented Sep 22, 2020

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-813

Checklist: I completed these to help reviewers :)

  • [ x] My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).

    No? -> read here on how to squash multiple commits into one

  • [x ] My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • [ x] I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • [x ] All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • [ x] My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@debicki
Copy link
Member

debicki commented Sep 22, 2020

It seemed to me that such tickets would be too easy for you. ;)

However, you forgot to add the "RESTWS-813:" tag to the commit description:
image

Additionally, I am curious what tests have you added? : P
From the description of PR:
image

@HerbertYiga
Copy link
Contributor

@insookwa Add a ticket Id to your commit,https://wiki.openmrs.org/display/docs/Pull+Request+Tips

@debicki
Copy link
Member

debicki commented Nov 20, 2020

@insookwa Are you still working on it?

@insookwa
Copy link
Author

oh, can I make another commit including the ticket ID to have this closed ? about the tests i added none @sacull

@insookwa
Copy link
Author

It seemed to me that such tickets would be too easy for you. ;)

However, you forgot to add the "RESTWS-813:" tag to the commit description:
image

Additionally, I am curious what tests have you added? : P
From the description of PR:
image

Thanks @sacull i have made a rebase and edited the commit message . please review and revert

@dkayiwa
Copy link
Member

dkayiwa commented Nov 21, 2020

review and revert

revert or merge 😊

@debicki
Copy link
Member

debicki commented Nov 21, 2020

i have made a rebase and edited the commit message

If so, only locally, because PR still contains commit from September. Additionally, there is some conflict, so you will have to resolve it.

@insookwa
Copy link
Author

review and revert

revert or merge

work in progress

@insookwa
Copy link
Author

i have made a rebase and edited the commit message

If so, only locally, because PR still contains commit from September. Additionally, there is some conflict, so you will have to resolve it.

@sacull please en light me about the conflict . how can i solve it

@debicki
Copy link
Member

debicki commented Nov 23, 2020

The easiest way to use Github.

image

Delete lines 1040, 1043-1045:
image

image
You'll probably get on with it. This is the easiest method, but not necessarily the best. ;)

Comment on lines 1040 to 1043

if (operationName.equals("post") || operationName.equals("get")) {
// createDefinition(operationEnum, resourceName, resourceParentName, representation);

createDefinition(operationEnum, resourceName, resourceParentName, resourceHandler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (operationName.equals("post") || operationName.equals("get")) {
// createDefinition(operationEnum, resourceName, resourceParentName, representation);
createDefinition(operationEnum, resourceName, resourceParentName, resourceHandler);
if (operationName.equals("post") || operationName.equals("get")) {
// createDefinition(operationEnum, resourceName, resourceParentName, representation);
createDefinition(operationEnum, resourceName, resourceParentName, resourceHandler);

Don't you think these blank lines are unnecessary?

Copy link
Member

@debicki debicki Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And is this code in the comment needed? Does it have any meaning for the source code?

@ibacher ibacher force-pushed the master branch 2 times, most recently from f9c73bb to 2a4407f Compare December 14, 2020 20:10
Comment on lines +1042 to +1045


if (operationName == "post" || operationName == "get") {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (operationName == "post" || operationName == "get") {

This fragment should be removed, otherwise, the code will not compile.

@dkayiwa dkayiwa closed this Mar 21, 2023
@njiddasalifu
Copy link

Hi @dkayiwa can I continue work on this issue. I see it cosed

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.

6 participants