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

Documentation of Server File Source Code #133

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

Conversation

jatin56
Copy link

@jatin56 jatin56 commented May 24, 2019

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

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.
Copy link
Member

@theSage21 theSage21 left a 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:

  1. Use docstrings in python
  2. Avoid adding comments explaining the code line by line
  3. Use sphinx to generate documentation from code

@jatin56
Copy link
Author

jatin56 commented May 24, 2019

Can you Tell me the reason For avoiding Comments,It would help me to improve from next time

@jatin56
Copy link
Author

jatin56 commented May 24, 2019

I have used Natural Docs instead of sphinx for creating Documentation from Code.
Created Documentation is present inside Docs Folder.

@theSage21
Copy link
Member

Can you Tell me the reason For avoiding Comments,It would help me to improve from next time

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?

  1. you've got the grammar of whatever language you are using

  2. names of functions/variables/classes

  3. the layout of the code

  4. x < 10? x+= 1: x += 2 and x += 1 if x < 10 else 2 do the same thing but the grammar of python makes the second one more readable and easily understood.

  5. This line in our project is an example of improper naming. That is not a test. The test is actually inside test[0]. Comments also form a part of this category.

  6. The last is the layout of the code. We are using black for that part. For example this 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.

  • clean code explains itself. It is obvious "what" it is doing
  • the code can not always explain "why" it is doing something. That's where comments come it.

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.

@theSage21
Copy link
Member

I have used Natural Docs instead of sphinx for creating Documentation from Code.
Created Documentation is present inside Docs Folder.

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?

Copy link
Member

@theSage21 theSage21 left a comment

Choose a reason for hiding this comment

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

  1. please keep review requests as small as possible. Makes it easier to review
  2. please use docstrings as mentioned in previous review

@jatin56
Copy link
Author

jatin56 commented May 25, 2019

I have used Natural Docs instead of sphinx for creating Documentation from Code.
Created Documentation is present inside Docs Folder.

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?

I find using Natural docs much easier to use than Sphinx . I Just followed the Coding Standard Used in
Natural Docs , Then it was just a one command to create Documentation . I have not used Sphinx much.

@rishabhKalakoti
Copy link
Contributor

@jatin56 Learning something new is always good XD

@jatin56
Copy link
Author

jatin56 commented May 25, 2019

  1. please keep review requests as small as possible. Makes it easier to review
  2. please use docstrings as mentioned in previous review

Can You Give me a Example ?, like what needs to be changed in the code documentation

@theSage21
Copy link
Member

well

  1. there are 83 file changes right now. I don't think I have the time to go through that
  2. this wiki explains what docstrings are please use those instead of random strings lying around the codebase

@theSage21
Copy link
Member

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'.

@jatin56
Copy link
Author

jatin56 commented May 25, 2019

Okay ,
I think it would be better if we would set up some kind of Standard for Documentation for this Project.
I got confused and used the standard i have been following. It would be beneficial for the people contributing in Future.

@theSage21
Copy link
Member

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.
@jatin56
Copy link
Author

jatin56 commented May 27, 2019

The changes are :
Replacing multi line commit with Docstrings
Merging new changes of master
Remove Docs Folder included Before

Copy link
Member

@theSage21 theSage21 left a 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):
"""
Copy link
Member

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:
"""
Copy link
Member

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):
"""
Copy link
Member

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):
"""
Copy link
Member

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):
"""
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

noise

def error404(error):
"""
Copy link
Member

Choose a reason for hiding this comment

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

noise

@jatin56
Copy link
Author

jatin56 commented Jun 15, 2019

All the Lines which you have commented ,nothing new can be removed,or i should explain more?

@theSage21
Copy link
Member

theSage21 commented Jun 15, 2019

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

@theSage21
Copy link
Member

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.

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.

None yet

3 participants