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

Updated readme.md as per requirements #196

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

Conversation

amanex007
Copy link

IMPORTANT NOTES (please read, then delete):

The PR title should start with "Fix #bugnum: " (if applicable), followed by a clear one-line present-tense summary of the changes introduced in the PR. For example: "Fix #bugnum: Introduce the first version of the collection editor.".

Please make sure to mention "#bugnum" somewhere in the description of the PR. This enables Github to link the PR to the corresponding bug.

Please also make sure to follow the style rules.

Issue [174](Cloud-CV#174) addressed and [179](Cloud-CV#179)
is also addressed.
removed spacing errors and changed the required words to "future"
@pushkalkatara
Copy link

Looks good to me!

@yashdusing
Copy link

yashdusing commented Dec 9, 2019

@amanex007 It would be great if you could link the screenshots along with the commands and the context under which the command has to be used. Make a separate document for this.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@amanex007 can you make the changes requested by @yashdusing . I am extending your task deadline by 1 day to accomodate these changes 🙂

@amanex007
Copy link
Author

@amanex007 can you make the changes requested by @yashdusing . I am extending your task deadline by 1 day to accomodate these changes slightly_smiling_face

already done

@amanex007
Copy link
Author

@amanex007 can you make the changes requested by @yashdusing . I am extending your task deadline by 1 day to accomodate these changes slightly_smiling_face

I have sent a document prepared by me through the portal.

@pushkalkatara
Copy link

pushkalkatara commented Jan 19, 2020

@amanex007 Can you please change the title of the PR according to the GCI issue,

I think this PR was for separate issues you worked on, the one i approved was for the previous commit. But, with the new commit in PR, I think the issue is also changed.

@pushkalkatara
Copy link

pushkalkatara commented Jan 19, 2020

I think you misunderstood the issue. Basically all the functionality implementation is in utils/auth.py and we are importing the functions and using it as a wrapper in the root directory for Click library.

The task was to implement code from L20 to L35 in utils/auth.py. You did right removing the replication, but the part mentioned above is left. Also, it would be better if you use os.makedir() in try-catch block just to be on a safer side.

Please feel free to ask if you have questions, also as @yashdusing said, please share the screenshots in the PR too.

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.

4 participants