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 ability to create gsoc page from data.yaml file #111

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

Conversation

pranshukharkwal
Copy link
Contributor

Solves #110
@vsvipul

Copy link
Member

@abhigyank abhigyank left a comment

Choose a reason for hiding this comment

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

Hi! Good work on this. Would it better to replace variable names like c, r,q,c1,afb,agh etc with like proper words? That may increase the readability of the code.
Like c4 is probably column4 ? afb with probably facebook-link and so on ?

@pranshukharkwal
Copy link
Contributor Author

@abhigyank I would rather replace c4 with row4 because it denotes the number of rows with four elements

@abhigyank
Copy link
Member

abhigyank commented Jun 28, 2020

Sounds good. I couldn't understand what c4 was, so I guessed it would be column4. It would therefore definitely help to have well worded variable names. If it denotes "number of rows with four elements" why not name the variable as four_elements_rows or rows_with_four_elements? Such naming would be more clearer.

@pranshukharkwal
Copy link
Contributor Author

@abhigyank I have changed variable names and added comments. Let me know if the code is understandable now?

@abhigyank
Copy link
Member

Much better. Leaving it to @vsvipul to review :)

@kpbot
Copy link
Member

kpbot commented Oct 1, 2020

Heads up @pranshukharkwal, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants