openmrs-core icon indicating copy to clipboard operation
openmrs-core copied to clipboard

Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

Open subhamkumarr opened this issue 1 year ago • 15 comments

Description of what I changed

Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

Issue I worked on

https://openmrs.atlassian.net/browse/TRUNK-6124

Checklist: I completed these to help reviewers :)

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

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

  • [X] All new and existing tests passed.

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

subhamkumarr avatar Jan 17 '24 05:01 subhamkumarr

@subhamkumarr change the title to reflect what's being done in the ticket

tendomart avatar Jan 17 '24 05:01 tendomart

@tendomart added

subhamkumarr avatar Jan 17 '24 06:01 subhamkumarr

@tendomart should i also add test to the code?

import org.junit.Test; import static org.junit.Assert.*;

public class ResultTest {

@Test
public void testToDatetimeWithNonNullValueDatetime() {
    // Create a Result with a non-null valueDatetime
    Result result = new Result(new Date(), Result.Datatype.DATETIME, null, null, new Date(), null, null, null);

    // Call toDatetime and assert the expected result
    assertEquals(result.getValueDatetime(), result.toDatetime());
}

@Test
public void testToDatetimeWithTextValue() {
    // Create a Result with datatype TEXT and a non-null valueText that can be parsed into a date
    String dateString = "2022-01-01";
    Result result = new Result(new Date(), Result.Datatype.TEXT, null, null, null, null, dateString, null);

    // Call toDatetime and assert the expected result
    assertEquals(DateUtils.parseDate(dateString), result.toDatetime());
}

@Test
public void testToDatetimeWithTextValueParsingError() {
    // Create a Result with datatype TEXT and a valueText that cannot be parsed into a date
    Result result = new Result(new Date(), Result.Datatype.TEXT, null, null, null, null, "invalidDate", null);

    // Call toDatetime and assert that it returns null
    assertNull(result.toDatetime());
}

@Test
public void testToDatetimeWithMultipleResults() {
    // Create a Result with multiple sub-results, each having a different valueDatetime
    Result subResult1 = new Result(new Date(), Result.Datatype.DATETIME, null, null, new Date(), null, null, null);
    Result subResult2 = new Result(new Date(), Result.Datatype.DATETIME, null, null, DateUtils.addDays(new Date(), 1), null, null, null);
    Result result = new Result(Arrays.asList(subResult1, subResult2));

    // Call toDatetime and assert that it returns the earliest valueDatetime among sub-results
    assertEquals(subResult1.toDatetime(), result.toDatetime());
}

}

subhamkumarr avatar Jan 17 '24 06:01 subhamkumarr

Yes please, just to make sure nothing is breaking.

tendomart avatar Jan 17 '24 06:01 tendomart

@tendomart Sir everything is good from my end. Please merge it.

subhamkumarr avatar Jan 17 '24 09:01 subhamkumarr

yes @subhamkumarr line 25

import static org.junit.Assert.*;

avoid imports like which end with .*, always import a specific class that you're using, avoid importing an entire package

look at this https://wiki.openmrs.org/display/docs/Java+Conventions

in short , use onlly absolute paths

tendomart avatar Jan 18 '24 06:01 tendomart

@tendomart done the changes. Could you please review and merge.

subhamkumarr avatar Jan 18 '24 14:01 subhamkumarr

@subhamkumarr have you responded to all the changes which I requested above ?

tendomart avatar Jan 19 '24 10:01 tendomart

@tendomart I made all the changes you asked me to do. Please review.

subhamkumarr avatar Jan 19 '24 10:01 subhamkumarr

@tendomart is this good for merge?

subhamkumarr avatar Jan 19 '24 10:01 subhamkumarr

@dkayiwa could you please review my PR.

subhamkumarr avatar Jan 19 '24 11:01 subhamkumarr

You have some failing tests..

do you need help in that ?


 Error:  Errors: 
Error:    AllergyValidatorTest.validate_shouldFailIfAllergenIsNull:82 » NullPointer Allergen must not be null
Error:    AllergyValidatorTest.validate_shouldFailIfPatientIsNull:73 » NullPointer Allergen must not be null
Error:    AllergyValidatorTest.validate_shouldRejectNumericReactionValue:191 » NullPointer Allergen must not be null

tendomart avatar Jan 19 '24 12:01 tendomart

@tendomart yes i need help

subhamkumarr avatar Jan 19 '24 12:01 subhamkumarr

@tendomart what changes i need to make?

subhamkumarr avatar Jan 19 '24 14:01 subhamkumarr

tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed.

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare!

This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side. Please note that this is an automated message and we might very well be the reason why there has not been any activity lately. We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs.

If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :) If you would like to continue working on it or require help from us please remove the stale label and respond by commenting on the issue.

github-actions[bot] avatar Aug 04 '24 00:08 github-actions[bot]