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

Logging Feature #72

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

Logging Feature #72

wants to merge 11 commits into from

Conversation

sgrewal12
Copy link

@sgrewal12 sgrewal12 commented May 1, 2018

Description

Added logging feature to track major search.

Pls do

  • Tested and working
  • Unit tests

Test plan

screen shot 2018-04-30 at 8 25 51 pm

@sgrewal12 sgrewal12 changed the title Sg logging Sg--logging May 1, 2018
@sgrewal12 sgrewal12 requested a review from marktai May 1, 2018 04:42
@sgrewal12 sgrewal12 changed the title Sg--logging Logging Feature May 1, 2018
Copy link
Collaborator

@marktai marktai left a comment

Choose a reason for hiding this comment

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

some nits.

also, does your editor remove trailing whitespace on the end of files? it's not a huge deal, but typical styleguides say that files have trailing newlines. It's actually a Linux standard and a lot of command line utilities dont deal with files correctly if you dont have a trailing newline

from rest_framework.response import Response
from .models import Data

def logger(cls, data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you name this log so the function is a verb and not a noun?

from .models import Data

def logger(cls, data):
new_data = Data(data_type=cls, log=data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the field name is data_type, can you name this variable to be data_type instead of cls

log = JSONField()

def __str__(self):
return '%s: %s' % (self.data_type, self.log)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might want to include the time on the str

@@ -189,4 +188,4 @@
REQUEST_TEMPLATE = '682f1eee-9441-4baf-aa1b-780281f25694'
USER_VERIFICATION_TEMPLATE = 'ddd14008-25ee-46c6-9a55-b6b4d577a54b'
MESSAGING_TEMPLATE = 'ea29ebe3-df27-4383-9234-0d01539980e3'
PASSWORD_RESET_TEMPLATE = '262bdea5-7921-47b1-946d-7438ee7f865b'
PASSWORD_RESET_TEMPLATE = '262bdea5-7921-47b1-946d-7438ee7f865b'
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this change be in this pull request?

@sgrewal12
Copy link
Author

sgrewal12 commented May 10, 2018

Did the changes, let me know if there is anything else that needs to be looked at. I'll look into the trailing newlines as well

return '%s: %s %s' % (self.data_type, self.log, datetime.now(pytz.timezone('US/Pacific')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

i meant date_created

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I figured that out later.. Changed it

@DavidWChen DavidWChen mentioned this pull request Dec 3, 2018
@marktai
Copy link
Collaborator

marktai commented Feb 28, 2019

oh man this PR is alive again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants