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

history.js - improving performance #15300

Merged

Conversation

Projects
None yet
5 participants
@Bournvita1998
Copy link
Contributor

commented Jun 3, 2019

Signed-off-by: Mohit Kuri mohit.kuri@research.iiit.ac.in

Description

Improved some performance factors present in history.js

Fixes #

Before submitting the pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

Bournvita1998 added some commits Jun 3, 2019

resolving some issues which I realised while creating PR
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
@phpmyadmin-bot

This comment has been minimized.

Copy link

commented on 0f3089e Jun 3, 2019

This commit is missing Signed-Off-By line to indicate that you agree with phpMyAdmin Developer's Certificate of Origin. Please check contributing documentation for more information.

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@mauriciofauth @williamdes
Improved some performance factors in histroy.js, Should I commit further updates in history.js over here only or should I create a new PR?

cc- @ibennetch

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@williamdes you can refer this for the previous discussion.

@mauriciofauth
Copy link
Member

left a comment

You should resolve the conflicts before we can merge your changes.

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Can you change your signature tag to use your full name?
We require a full name for legal compliance reasons.

For example: Signed-off-by: Jane Smith <jane.smith@example.org>

See Developer's Certificate of Origin

@williamdes
Copy link
Member

left a comment

LGTM @Bournvita1998

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

You should resolve the conflicts before we can merge your changes.

I couldn't figure out what's the exact conflict. Can you please help me here?

@mauriciofauth
Copy link
Member

left a comment

You should fix the ESLint error.
See: https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/541106844#L402

You can run ESLint with:

node_modules/.bin/eslint js/designer/history.js

Bournvita1998 added some commits Jun 5, 2019

Reverting a few changes
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
fixing ESLint error
Signed-off-by: Mohit Kuri <mohit.kuri@research.iiit.ac.in>
@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@mauriciofauth fixed...

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@mauriciofauth Can you please review this?
And if everything is good... should I go ahead and create a new PR "Refactoring in history.js" or should I commit those changes here in the same PR?

cc- @williamdes @ibennetch

Show resolved Hide resolved js/designer/history.js Outdated
Show resolved Hide resolved js/designer/history.js Outdated
@shucon

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@Bournvita1998 have you even tested this PR? This code won't even work.
@williamdes @mauriciofauth please don't merge the code yet.

@mauriciofauth
Copy link
Member

left a comment

There are still ESLint errors.

You can run ESLint with:

node_modules/.bin/eslint js

or

yarn run js-lint
@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth @shucon I think while creating this PR I messed up at a lot of points(worked on different branch initially, while copying changes to the new branch got a lot of indentation issues, pulled the changes late, merge conflict, etc... ) which lead to soo many errors. I was thinking of creating a new PR, should I create one soon? I have already done work on that but now looking that how can I test that(posted the query in the mailing list regarding the same).

PS- I have considered your reviews while working this time.

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@Bournvita1998 The majority of issues is just name changes. For example, you are using history_array but that variable is now named historyArray.

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

These conflicts were caused by some recent changes I made in the JavaScript files.

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth ohh okay, So I guess it's okay if I resolve the issues to this PR only?

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

You can keep working on this PR.

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth while resolving ESLint error, I got these 3 errors along with other 183 warnings. Should I rename the variables or leave them as it is?

Selection_042

Changing the variable names and resolving other issues
Signed-off-by: Bournvita1998 <mohit.kuri@research.iiit.ac.in>
Show resolved Hide resolved js/designer/history.js Outdated
Show resolved Hide resolved js/designer/history.js Outdated
Show resolved Hide resolved js/designer/history.js Outdated
Show resolved Hide resolved js/designer/history.js Outdated
changing the signatures
Signed-off-by: Mohit Kuri <mohit.kuri@research.iiit.ac.in>
@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth Resolved the changes suggested

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth Should I now move ahead to refactor history.js?

@mauriciofauth

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Should I now move ahead to refactor history.js?

@Bournvita1998 Yes, you can open a new pull request if you want.

@Bournvita1998

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@mauriciofauth Any suggestion on the query which I asked on the developer's list and also in our personal gitter conversation?

@mauriciofauth mauriciofauth merged commit 36af797 into phpmyadmin:gsoc-db-designer Jun 7, 2019

0 of 2 checks passed

DCO DCO
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@mauriciofauth mauriciofauth self-assigned this Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.