Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enh/utc time #110

Merged
merged 18 commits into from
Jan 9, 2022
Merged

Enh/utc time #110

merged 18 commits into from
Jan 9, 2022

Conversation

FranzYuri
Copy link
Contributor

@FranzYuri FranzYuri commented Nov 9, 2021

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • Code docs are working correctly
  • Code base maintenance (refactoring, formatting, renaming):

    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally
  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

Currently the output dates on RocketPy are only available in UTC time zone.

What is the new behavior?

Now RocketPy can print dates on local time zones.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Tanks to @ompro07 for all help, especially with the contributions at https://github.com/Projeto-Jupiter/Hackathon/pull/97/files

@giovaniceotto
Copy link
Member

Haven't taken a complete look at this PR just yet, but it seems like the tests are failing because of the pytz library.

@FranzYuri, can you modify the requirements.txt file in the root folder and add pytz as a required module?

Let me know if you have any doubts.

@Gui-FernandesBR
Copy link
Member

@giovaniceotto , as requested for you, the requirements.txt are now updated. Can you please confirm that everything is in the right format? This file is not familiar for me, but I think that maybe we need specify package version, right?

@giovaniceotto
Copy link
Member

I am sorry I haven't had time to review this yet. I will need a little more time since this uses a library that I am not familiar with.
Help from @Lucas-KB and @ompro07 would be awesome.

@Gui-FernandesBR
Copy link
Member

@Lucas-KB , could you please take a look at this pull request for us?

Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Took a while, but I finally got to reviewing this masterpiece! Magnificent work @FranzYuri and @ompro07!

I made some comments, suggestions and raised a question. Let me know what you think. If needed, we can discuss further via discord.

rocketpy/Environment.py Outdated Show resolved Hide resolved
rocketpy/Environment.py Outdated Show resolved Hide resolved
rocketpy/Environment.py Outdated Show resolved Hide resolved
rocketpy/Environment.py Outdated Show resolved Hide resolved
@giovaniceotto
Copy link
Member

@FranzYuri, I chipped in and did some minor code refactoring. Here are the most significant changes I have made:

  • Introduced the time_zone argument in the Environment.__init__ constructor. I did this since it is also possible to set the datetime in this constructor, so I believe it would be important to have the time zone there as well.
  • Changed the way that the local time is converted to UTC. Before, we had
    self.date = self.localDate.replace(tzinfo=pytz.UTC) which I modified to self.date = self.localDate.astimezone(pytz.UTC). This converts the local date to UTC, which is what RocketPy needs to correctly interact with weather models.
  • Minor changes to how the time is printed. Now, both time zones are shown in the same line.
  • Minor code refactoring, converting snake_case to camelCase since we are still using this standard. This should be reverted when we release v1.0.0.

I believe there are only two things left:

  • Create a test for this new feature.
  • Add an example of this new feature to the notebook at RocketPy/docs/notebooks/environment_class_usage.ipynb.

Do you wish to work on these items on this PR? Otherwise, we can merge this PR after you review my changes and then create a new one focused on the two items left.

@giovaniceotto giovaniceotto self-requested a review January 3, 2022 15:24
@Gui-FernandesBR
Copy link
Member

Hey, I'd like to recommend merging this PR and then start a new one including the examples.

@FranzYuri do u agree?

@FranzYuri
Copy link
Contributor Author

Hey, I'd like to recommend merging this PR and then start a new one including the examples.

@FranzYuri do u agree?

Yes! Sorry for the delay...

@FranzYuri FranzYuri merged commit 4caf4e0 into develop Jan 9, 2022
@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes NewFeature labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants