-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FEATURE]: Publish monthly a plot on Twitter with the gas price variation #9 #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great.
I have fewer sugestions of things we can do to further improve it, either now or in future development.
- Do we really need to save the plots as a picture in out git repository, or can we just have an image object that we can upload to Twitter straight in the github action?
- Would be interesting in a futuredevelopment to add some styling to the plot with the python PIL library.
Very good and solid code. Approved.
Good job! @Dntfreitas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! Well done, @Dntfreitas ! 🎉
Just left a note according to the comment of @joaoofreitas.
A constant was included for completeness
Hey All, If you agree, I can merge this PR. Thanks for the feedback ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leave a note on a constant, not critical. Just take a look at it, and it's ready to merge!
@Dntfreitas let us know if we can merge this PR. If yes, you can do it directly. Let's clean the completed issues and PRs. |
Guys, please check if everything is okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Ready to merge IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @Dntfreitas ! 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good changes, except for one typo from the main
branch itself and the need to remove git add history/img/plot.png
from main.yml
.
@@ -49,8 +49,7 @@ | |||
# If we don't have the date, update | |||
if update: | |||
# Prepare the dictionaire | |||
dict_prices = {PREVIOUS_WEEK: curret_data[CURRENT_WEEK]} | |||
dict_prices[CURRENT_WEEK] = {START_DATE_KEY: start_date, END_DATE_KEY: end_date, GAS_KEY: {}} | |||
dict_prices = {PREVIOUS_WEEK: curret_data[CURRENT_WEEK], CURRENT_WEEK: {START_DATE_KEY: start_date, END_DATE_KEY: end_date, GAS_KEY: {}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is typo curret_data
, should be current_data
; this stems from here:
Lines 13 to 15 in 010431a
curret_data = json.load(f) | |
current_start_date_saved = curret_data[CURRENT_WEEK][START_DATE_KEY] | |
current_end_date_saved = curret_data[CURRENT_WEEK][END_DATE_KEY] |
@@ -33,5 +33,6 @@ jobs: | |||
git add gas_info.json | |||
git add history/gas_info_history.csv | |||
git add history/gas_info_history.json | |||
git add history/img/plot.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed since you are not writing the file on the repo any more.
@carlosrsabreu : Sorry, didn't realize this was merged while I went to have my dinner 🙁 |
Sorry, @HarryVasanth ! I though you saw before merged it 😔 You were checked in the reviewers' section |
What this PR does
Publishes, every month, a plot with the gas variation from the last 6 months.