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

datetime #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

datetime #53

wants to merge 3 commits into from

Conversation

guifelix
Copy link

@guifelix guifelix commented Mar 8, 2019

What type of change does this PR introduce?

  • Bugfix
  • [ x] Feature
  • Refactor
  • Documentation
  • Not Sure?

Does this PR introduce breaking changes?

  • Yes
  • [x ] No

List any relevant issue numbers:

Description:

A package deal with date and time

@louistiti
Copy link
Member

Hello @guifelix,

Thanks for contributing, it looks great via the code! 👍
Let me take a deeper look later on and come back to you.

@louistiti louistiti mentioned this pull request Mar 9, 2019
@louistiti
Copy link
Member

@louistiti
Copy link
Member

louistiti commented Mar 9, 2019

I guess the NLU improvement will help for that package (as Leon can extract date entities from the core and gives it to modules). You can take a look at this branch to have a better understanding. Or you can wait until I finish that improvement and once I wrote the proper documentation.

@louistiti
Copy link
Member

louistiti commented Apr 5, 2019

I've edited the docs according to the NLU improvements. You can take a look here 😉

Let me know if you have any questions.

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! @guifelix
I have a new idea for this package, we could ask "What time is it in city_name" for example "What time is it in San Francisco ?", so we could know the time in multiple time zone.

I submit several comments about your code, feel free to discuss.

@@ -7,6 +7,7 @@ name = "pypi"
requests = "==2.21.0"
pytube = "==9.2.2"
tinydb = "==3.9.0"
parsedatetime = "*"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think, it is great to use "*" for the version number because things could break between releases, so it would be better to have an exact version number.

...
```

## Todo
Copy link
Member

Choose a reason for hiding this comment

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

We should not use "Todo" inside markdown files, instead it would be GitHub issues or Trello Card.

What do you think?

import utils
import datetime
import parsedatetime
# import dateparser
Copy link
Member

Choose a reason for hiding this comment

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

Why there is this comment ?

if word.startswith('the'):
word = word.replace('the', 'one', 1)

cal = parsedatetime.Calendar()
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid abbreviations for variable names. So instead of cal, we could call it calendar.

random = randint(0, 2)

# https://stackoverflow.com/questions/9647202/ordinal-numbers-replacement
suf = lambda n: "%d%s"%(n,{1:"st",2:"nd",3:"rd"}.get(n if n<20 else n%10,"th"))
Copy link
Member

Choose a reason for hiding this comment

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

It is quite hard to understand, what this is trying to do, maybe we could refactor this in multiple functions/method or with variables etc.

Comment on lines +35 to +36
#trying to use dateparser lib
# return utils.output('end', 'guess', utils.translate('guess', { 'guess': dateparser.parse(word) }))
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid useless comments.

Comment on lines +23 to +25
hrs = time.hour
mins = time.minute
msg = ""
Copy link
Member

Choose a reason for hiding this comment

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

As I previously said, we should avoid abbreviations for variable names.


def saycurrenttime(string):

# https://sukhbinder.wordpress.com/2013/12/29/time-in-words-with-python/
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need this link.

@theoludwig theoludwig added the feature request Indicates new feature requests. label Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Indicates new feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants