-
Notifications
You must be signed in to change notification settings - Fork 29
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
Documentation of Server File Source Code #133
base: master
Are you sure you want to change the base?
Conversation
All the Classed included in server files are properly Documented. This will help to understand the properties of these model properly.
All the methods included in server files are properly Documented. This will help to understand the properties of these model properly.
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 good work 👍
To improve it we can do the following:
- Use docstrings in python
- Avoid adding comments explaining the code line by line
- Use sphinx to generate documentation from code
Can you Tell me the reason For avoiding Comments,It would help me to improve from next time |
I have used Natural Docs instead of sphinx for creating Documentation from Code. |
The way I see it, programming is the act of communicating ideas to other people because if it was about communicating with the machine we were doing fine with assembly. So what are we communicating with when we write code?
def __init__(self, *, use_dns_cache: bool=True, ttl_dns_cache: int=10, family: int=0, ssl: Union[None, bool, Fingerprint, SSLContext]=None, local_addr: Optional[Tuple[str, int]]=None, resolver: Optional[AbstractResolver]=None, keepalive_timeout: Union[None, float, object]=sentinel, force_close: bool=False, limit: int=100, limit_per_host: int=0, enable_cleanup_closed: bool=False, loop: Optional[asyncio.AbstractEventLoop]=None): is better written as aiohttp has done it since it's obviously clear where and what the arguments to the function are. Now that we understand that code is for communicating ideas to people, let's get to what comments are to be used for IMHO.
A good example would be something like this: connector = aiohttp.TCPConnector(ttl_dns_cache=None)
# by setting ttl_dns_cache as None(never refresh) we ensure that there is no memory leak which was
# otherwise occuring due to the ever growing _DNSCacheTable of the TCPConnector
# reference link - https://github.com/aio-libs/aiohttp/issues/3684 Here the code has no way to explain WHY that dns cache has to be None. Hence this comment is useful. Unless there is a WHY which needs to be explained, there is no need for a comment. Similarly comments may be used sometimes (I don't have enough experience doing this but I've seen other good people do it) to tell programmers that some parts of the code are off limits. Perhaps you need advanced mathematics to understand that part and it is easy to do wrong. In that scene you might put a comment saying something to the effect of "DO NOT TOUCH". There is a talk by Kevlin Henney where he talks about some of these things. |
please don't commit docs in the code. Docs are "generated" and so it's easier to host them on something like read the docs. I'm not familiar with natural docs. If it's not too much work can we use the standard Sphinx setup? |
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.
- please keep review requests as small as possible. Makes it easier to review
- please use docstrings as mentioned in previous review
I find using Natural docs much easier to use than Sphinx . I Just followed the Coding Standard Used in |
@jatin56 Learning something new is always good XD |
Can You Give me a Example ?, like what needs to be changed in the code documentation |
well
|
Natural docs would be a good choice for you if it was a personal project that you were working on. Here's why I don't like it: Compare that to sphinx. You can explore the insights part of those projects on your own. Since PyJudge is a community project + learning activity I'd like to avoid "one click solutions" since all you learn there is how to click rather than how the thing actually works. This is the kind of 'knowledge' that leads to absolute abominations like 'sophia the AI robot'. |
Okay , |
Yes. I added comments to the issue. I'm not sure if that is what you meant when you said standards for documentation. Is it? |
The documentation is done sing docstring instead of normal multi line strings.the documentaiion is now acording to the formats using for sphinx.
The changes are : |
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.
- if you have questions regarding the last review please ask them before submitting a new review
- a comment/doc should explain WHY something is happening as opposed to what is happening.
- some parts could be improved. I have mentioned those places in the review comments.
- sphinx was missing as far as I could tell
database = db | ||
|
||
|
||
class Session(Model): | ||
""" |
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.
see, this comment does not tell me anything new. What it says I can read in the code. A good docstring would let me know WHY the string being random is necessary.
server.py
Outdated
username = CharField(unique=True) | ||
password = CharField() | ||
|
||
class Meta: | ||
""" |
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's nothing new in this comment. we can safely remove it
server.py
Outdated
class User(Model): | ||
""" |
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.
nothing new in this comment. we can safely remove it.
server.py
Outdated
@@ -33,6 +43,12 @@ class Meta: | |||
|
|||
|
|||
class Contest(Model): | |||
""" |
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.
nothing new
server.py
Outdated
@@ -43,6 +59,9 @@ class Meta: | |||
|
|||
|
|||
class Question(Model): | |||
""" |
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.
nothing new
server.py
Outdated
@@ -329,26 +494,35 @@ def file_upload(code, number): | |||
return bottle.abort(404, "no such contest problem") | |||
user = Session.get(Session.token == bottle.request.get_cookie("s_id")).user | |||
time = datetime.datetime.now() | |||
uploaded = bottle.request.files.get("upload").file.read() | |||
uploaded = bottle.request.files.get("upload").file.read() # read the uploaded File |
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.
noise
try: | ||
Submission.create( | ||
user=user, contestProblem=contestProblem, time=time, is_correct=ans | ||
) | ||
)# Add Submission For User |
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.
noise
except: | ||
bottle.abort(500, "Error in inserting submission to database.") | ||
if not ans: | ||
|
||
if not ans: # if Answer is Wrong |
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.
noise
server.py
Outdated
return "Wrong Answer!!" | ||
else: | ||
return "Solved! Great Job! " | ||
|
||
|
||
@app.error(404) | ||
|
||
@app.error(404) # Checks if app returns an error with error code as argument |
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.
noise
def error404(error): | ||
""" |
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.
noise
All the Lines which you have commented ,nothing new can be removed,or i should explain more? |
I meant remove the comment/docstring itself. There are around 30 ish things that can be removed since they are adding no value. I don't remember exactly since this was almost one month back |
This commit has been open for a month now without necessary feedback being implemented. I'd like to keep it open in case you're still working on the changes, but if you're exploring the codebase I'd like to close this PR and let someone else try their hand at the issue. I'll be keeping this open till the end of week for the required changes to go in. Then I'll be closing it. |
I Have Documented the code present in Server.py file.
I tried to maintain a proper standard while documentation.
The documentation of code is done with my understanding of code and may require improvements or #132