-
Notifications
You must be signed in to change notification settings - Fork 209
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.
Could you squash the first two commits? They seem to represent one feature but are disconnected
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.
I will run this locally and we should be good for merge then 💯
c9ce4bb
to
aa5a4f2
Compare
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.
All in all no major changes from my side.
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.
Make sure to squash your commits!
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.
Some more comments.
As for the location data can you move it to a dedicated folder like locationdata/
instead of keeping at the top level?
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.
LGTM. @SanketDG @sakshi1499 ?
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.
LGTM
Description
Fixes #542
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
Tested Locally
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only