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

Add pretty format for command 'get_token' #195

Closed
wants to merge 2 commits into from

Conversation

m-sameer
Copy link

@m-sameer m-sameer commented Dec 8, 2019

Fix #181: Add pretty format for command 'get_token'

Before:
Screenshot from 2019-12-08 20-46-39
After:
Screenshot from 2019-12-08 19-36-42

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.

@m-sameer can you try to figure out a more generalized method to pretty print CLI outputs? Like in this case we are hardcoding the color of the token can we do it in a more generalized manner

@m-sameer
Copy link
Author

m-sameer commented Dec 8, 2019

I would suggest that we build a colour palette with a string having a certain colour, a link having a certain colour, names of challenges, date, username etc.

What about that?

@pushkalkatara
Copy link

@m-sameer Yes, that's right. We all can have a discussion and decide a color.

@m-sameer
Copy link
Author

m-sameer commented Dec 8, 2019

Yes.

Links can be blue. The classics way.
Token (and other strings) be green.

@pushkalkatara
Copy link

To manage the discussion we can list down commands which need to be designed. Blue links and green tokens would suite best.

Also, as @Ram81 added that we would need a more generalized manner and not hardcoded colors. To understand this context please have a look at challenge table design in which a single function is used to summarize all the challenge related CLI commands.

@m-sameer
Copy link
Author

m-sameer commented Dec 8, 2019

So what do I do?

Have some global variables (of colours) for different information?

@yashdusing yashdusing mentioned this pull request Dec 9, 2019
@m-sameer
Copy link
Author

m-sameer commented Dec 9, 2019

@pushkalkatara How should I proceed further?

@pushkalkatara
Copy link

Yes, as in the challenge.py there is single function for all the challenge related commands. You can create for other files and commands as well. I have extended the time on GCI dashboard.

@m-sameer
Copy link
Author

Where do I store the variables?

@m-sameer m-sameer force-pushed the issue-181 branch 4 times, most recently from f1b9d46 to 25e75d3 Compare December 18, 2019 19:01
@m-sameer
Copy link
Author

@pushkalkatara Please review.

@pushkalkatara
Copy link

@m-sameer Do we need a class to store variables?

@m-sameer
Copy link
Author

We need a class to tell of what category the variables belongs to.

@pushkalkatara
Copy link

Can we think of other ways? Maybe a dictionary?

@m-sameer
Copy link
Author

Why a dictionary though?

@m-sameer
Copy link
Author

@pushkalkatara Please review.

@pushkalkatara
Copy link

Hi @m-sameer Sorry for the late response.

Here's the expectation from this issue:

  1. There are several commands here in https://github.com/Cloud-CV/evalai-cli/blob/master/evalai/utils
  2. As you have chosen token related commands i.e. auth commands which are under the utils here - https://github.com/Cloud-CV/evalai-cli/blob/master/evalai/utils/auth.py
  3. You have to make changes to this file, write a function such as pretty_print_auth_commands or whatever name you like. This would generalize a function for all auth related commands.

Actually I just found that there is some replication in the code in evalai-cli/get_token.py and evalai-cli/utils/auth.py. I have created a separate issue to remove this replication #223 .

@m-sameer
Copy link
Author

@pushkalkatara What do I do in that function?

Copy paste this function?

@m-sameer
Copy link
Author

@pushkalkatara please reply

@pushkalkatara
Copy link

@m-sameer Create a function here named as pretty_print_auth_commands with necessary arguments just like here for challenges. And use this function in each auth related function in auth.py to pretty print on terminal.

I apologize for the late reply. I have been traveling recently.

@m-sameer m-sameer force-pushed the issue-181 branch 2 times, most recently from 11f03f2 to 2f1f158 Compare December 28, 2019 14:31
evalai/utils/auth.py Outdated Show resolved Hide resolved
@pushkalkatara
Copy link

For now, it looks good and we can approve the GCI task. I think we can more generalize the pretty print commands format by using it for other CLI commands too. One liner function doesn't look good, maybe when we extend it for other CLI commands, it would have a better format. What do you guys think @Ram81 @vkartik97

@m-sameer
Copy link
Author

Well, I got screwed here.
This PR wasn't scrutinised as much as mine was.
Probably it's a GCI task PR.

@m-sameer
Copy link
Author

@pushkalkatara Please reply.

@pushkalkatara
Copy link

@m-sameer Don't worry. This PR got a bit delayed, you did a good job but the expectations changed with time (like we discovered a new issue that there is code replication, also we discovered that we can more generalize the pretty-print function). I apologize if I was late with the response.

I am approving your GCI task as you have solved the issue and helped us to discover more issues. We'll keep the PR on hold for a bit until we resolve the replication of get_token command. As this PR and GCI task has several instances, and you are also familiar with the code, you can retake another pretty print functionality if you're interested.

@m-sameer
Copy link
Author

@pushkalkatara Well I am having my feet cut off because of the extreme delay and that takes my 30 days from the contest.

I can never ever win this contest because of this single PR.

@pushkalkatara
Copy link

@m-sameer Hey don't worry you still have a lot of time and there are plenty of tasks. Keep your spirits up and make most learning out of GCI.

@m-sameer m-sameer closed this May 27, 2020
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.

[GCI-2019] Pretty print CLI output on terminal.
4 participants