Code for moving data files#578
Conversation
nworden
left a comment
nworden
left a comment
There was a problem hiding this comment.
Thanks for the new PR! It looks like this includes a lot of changes unrelated to the data files though; did you mean to include them in this PR?
|
|
||
| import config | ||
| import const | ||
| from utils import const |
There was a problem hiding this comment.
Did you intend to include all these changes related to const.py in this PR (the title says it's just for moving the data files)?
There was a problem hiding this comment.
The diffs I'm looking at are your current PR compared to our master branch, and there are a lot of changes unrelated to the data files. In particular, in many places it changes imports of const from import const to from utils import const (along with changes to sys.path and the django.po files).
There was a problem hiding this comment.
Did you intend to include all these changes related to const.py in this PR (the title says it's just for moving the data files)?
@nworden It was actually the part of handlers which were supposed to be moved from app directory. Now since handlers are kept under app/ we can keep const.py here itself.
There was a problem hiding this comment.
The diffs I'm looking at are your current PR compared to our master branch, and there are a lot of changes unrelated to the data files. In particular, in many places it changes imports of
constfromimport consttofrom utils import const(along with changes to sys.path and the django.po files).
@nworden I have reverted that change to import const
| msgstr "Kontakinligting" | ||
|
|
||
| #, python-format | ||
| msgid "Content purported to be compressed with %s but failed to decompress." |
There was a problem hiding this comment.
I'm not sure exactly why, but it looks like messages from files in app/vendors is getting added to the django.po files. That's not necessary -- we only need our own messages in here.
There was a problem hiding this comment.
Even am trying to understand how messages are getting added to django.po files. Neither any of regular expression in my code is appending so. What exactly is the ask here ?
There was a problem hiding this comment.
Please undo the changes you made to these files, we can't pull them into master. If you committed just the django.po file changes by themselves in a particular commit, it might be as easy as running git revert <that commit>.
| See here for usage examples: | ||
| https://github.com/google/personfinder/wiki/DeveloperFaq | ||
| """ | ||
| import sys |
There was a problem hiding this comment.
I'm not sure exactly what this is for (?), but it shouldn't be necessary to modify sys.path within our code.
There was a problem hiding this comment.
@nworden sys.path was present since directory named handlers and utils were having config.py in it and it was supposed to be imported.
There was a problem hiding this comment.
I think you can remove the sys import as well.
|
|
||
| import datetime | ||
| import unittest | ||
| # added by Ashutosh Narayan |
There was a problem hiding this comment.
Although there are names from the earlier Person Finder authors still in the codebase, in recent years we haven't been adding individual names to the source code (I think I've only seen gimite's name once or twice, and I've never added my name anywhere); the commit log is the record of who did what. If you'd like us to start an AUTHORS file, feel free to open an issue for that and assign it to me (Google might have rules about how exactly I do it, I'd have to check), but I don't think it's practical to have names inline in the source considering how many people have contributed different pieces over time.
There was a problem hiding this comment.
@nworden I have removed name from source code ; it was added as part of testing to remove later if there are any failures due to lines appended by me. Yes, we can have an AUTHORS file
|
|
||
| import config | ||
| import const | ||
| from utils import const |
There was a problem hiding this comment.
The diffs I'm looking at are your current PR compared to our master branch, and there are a lot of changes unrelated to the data files. In particular, in many places it changes imports of const from import const to from utils import const (along with changes to sys.path and the django.po files).
| import logging | ||
| import datetime | ||
| import utils | ||
| #import utils |
There was a problem hiding this comment.
I think you may have accidentally commented this import out.
| See here for usage examples: | ||
| https://github.com/google/personfinder/wiki/DeveloperFaq | ||
| """ | ||
| import sys |
There was a problem hiding this comment.
I think you can remove the sys import as well.
| msgstr "Kontakinligting" | ||
|
|
||
| #, python-format | ||
| msgid "Content purported to be compressed with %s but failed to decompress." |
There was a problem hiding this comment.
Please undo the changes you made to these files, we can't pull them into master. If you committed just the django.po file changes by themselves in a particular commit, it might be as easy as running git revert <that commit>.
|
|
||
| """Tests for utils.""" | ||
|
|
||
| import sys |
There was a problem hiding this comment.
As with the other file, please remove any changes to the path.
2 participants
Code for moving data files from app/ directory