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

♻️ Refactor Functions #344

Merged
merged 15 commits into from
Nov 27, 2019
Merged

♻️ Refactor Functions #344

merged 15 commits into from
Nov 27, 2019

Conversation

athul
Copy link
Contributor

@athul athul commented Nov 26, 2019

Make use of ES6+ constructs

  • Arrow functions
  • Template literals
  • Object destructuring (concise code)

@ghost
Copy link

ghost commented Nov 26, 2019

DeepCode Report (#d739af)

DeepCode analyzed this pull request.
There is 1 new info report. 1 info report was fixed.

assets/js/curlparser.js Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 27b2a740-104c-11ea-a65a-5f455ab5a299

@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 3d227a10-104c-11ea-a65a-5f455ab5a299

@athul
Copy link
Contributor Author

athul commented Nov 26, 2019

@NBTX Done ✨ ✨

@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 936f1bc0-104d-11ea-a65a-5f455ab5a299

Copy link
Member

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

Left couple of comments

assets/js/curlparser.js Outdated Show resolved Hide resolved
components/collections/editRequest.vue Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 4cf755c0-104f-11ea-a65a-5f455ab5a299

@jamesgeorge007 jamesgeorge007 added the refactor Code refactoring label Nov 26, 2019
Co-Authored-By: James George <jamesgeorge998001@gmail.com>
@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 173ea130-1050-11ea-a65a-5f455ab5a299

pages/realtime.vue Outdated Show resolved Hide resolved
@athul
Copy link
Contributor Author

athul commented Nov 26, 2019

@jamesgeorge007 Done I guess 😂

Thanks for all your mentoring folks. Funny thing is IDK Vue and Js. I've always wanted to Help

@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: ad997720-1052-11ea-a65a-5f455ab5a299

@athul
Copy link
Contributor Author

athul commented Nov 26, 2019

I guess that most of the Functions refactored on .vue files and I guess thats why all of em are failing(tests). I think this should be closed as of now and I'll create a new PR with just the .js files refactored.

How does that sound?

@athul
Copy link
Contributor Author

athul commented Nov 26, 2019

What do you say @liyasthomas @NBTX @jamesgeorge007
Close it, if you feel like this should be closed

@liyasthomas
Copy link
Member

@athul no need. I'm quite busy with office stuffs rn, will be able to make this workable with some tweaks.

@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: dded9af0-1061-11ea-a65a-5f455ab5a299

Co-Authored-By: James George <jamesgeorge998001@gmail.com>
@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: bd6677b0-1062-11ea-a65a-5f455ab5a299

pages/index.vue Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Hey @athul,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 4d553d80-10ad-11ea-a65a-5f455ab5a299

@TravisBuddy
Copy link

Hey @athul,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 93979010-10b5-11ea-a65a-5f455ab5a299

@liyasthomas
Copy link
Member

@jamesgeorge007 please take a look at this PR. Contribute to this pull if there's any refactoring left.

@liyasthomas liyasthomas added the need testing Needs to be tested before merging onto production label Nov 27, 2019
Copy link
Member

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

Just a couple of more tweaks remaining.

assets/js/curlparser.js Outdated Show resolved Hide resolved
assets/js/curlparser.js Outdated Show resolved Hide resolved
assets/js/curlparser.js Outdated Show resolved Hide resolved
Copy link
Member

@liyasthomas liyasthomas left a comment

Choose a reason for hiding this comment

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

🆗

@TravisBuddy
Copy link

Hey @athul,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d2be8d70-110a-11ea-b8ff-d5ab8a371226

@TravisBuddy
Copy link

Hey @athul,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 26d25620-110c-11ea-b8ff-d5ab8a371226

@liyasthomas liyasthomas self-requested a review November 27, 2019 11:51
@liyasthomas liyasthomas merged commit 4f5788f into hoppscotch:master Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need testing Needs to be tested before merging onto production refactor Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants