Adding ESlint #37
Merged
Adding ESlint #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Enable as many of them as you can work with, even if it's a bit uncomfortable at first and needs some refactoring. This will should ensure good code quality of this lib in the long run. |
|
Yes, when I looked at the changes I made, I couldn't find the disabled
rules, if there were any. They are mostly at the top of the marked file in
comments right?
…On Tue, Jun 18, 2019, 10:12 AM Pranshu Srivastava ***@***.***> wrote:
Enable as many of them as you can work with, even if it's a bit
uncomfortable and needs some refactoring. This will should ensure good code
quality of this lib in the long run.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AJXHQZ7UUMVRU3QHV6XSSVLP3BRTBA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5FEFA#issuecomment-502944276>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ4FWZAARVLQ2RAPOSTP3BRTBANCNFSM4HWIVW6Q>
.
|
|
Could be. Could be inside the |
|
Yes, I checked, wasn't there. So this might be good to merge?
…On Tue, Jun 18, 2019, 11:35 AM Pranshu Srivastava ***@***.***> wrote:
Could be. Could be inside the .eslintrc too.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AJXHQZ2B4VOLPARD3UUQB73P3B3JXA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5JCPY#issuecomment-502960447>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZZ4OP5WP6JULKWP3TLP3B3JXANCNFSM4HWIVW6Q>
.
|
|
Just to confirm, you guys never really linted the code, did you? |
|
Because I cannot see any eslint ignore rules as of now in any of the files. |
|
Yes, I tried it locally I think, but never really linted it, as far as I
remember. You're right, there are no rules there.
…On Tue, Jun 18, 2019, 11:43 AM Pranshu Srivastava ***@***.***> wrote:
Because I cannot see any eslint ignore rules as of now in any of the files.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AJXHQZYVCNBQG3QCRWGZNPTP3B4JJA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5JSDA#issuecomment-502962444>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ2UGT3NC4QZBE2JRILP3B4JJANCNFSM4HWIVW6Q>
.
|
|
Maybe we should wait for @namangupta01's review here, or do you believe we should proceed with the merge? |
|
I think there's not much to discuss in this one so we can proceed with the
merge. What do you think? Maybe we should wait for @jywarren 's approval?
…On Tue, Jun 18, 2019, 12:12 PM Pranshu Srivastava ***@***.***> wrote:
Maybe we should wait for @namangupta01 <https://github.com/namangupta01>'s
review here, or do you believe we should proceed with the merge?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AJXHQZ6H5VHXBLHVWL3FMYTP3B7W5A5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5LKLA#issuecomment-502969644>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZYWYQETSMKBCN3I3LLP3B7W5ANCNFSM4HWIVW6Q>
.
|
|
Thanks a lot! :D
…On Tue, Jun 18, 2019, 7:42 PM Jeffrey Warren ***@***.***> wrote:
***@***.**** approved this pull request.
:-) very nice!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#37?email_source=notifications&email_token=AJXHQZ3R6NK435CS5MTNZLTP3DUL3A5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3354AQ#pullrequestreview-251125250>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ4MD5WQYQF4ZGX5GSDP3DUL3ANCNFSM4HWIVW6Q>
.
|
IshaGupta18
added a commit
that referenced
this pull request
Aug 10, 2019
* Project Setup (#5) * PublicLab Grapher Project Setup * Added Jquery, Papaparse and chart.js dependencies in package.json (#7) * Adding the parsing feature according to new project structure * Added npm run script to watch changes and added browserify package for build (#10) * Added Mocha testing framework and chai for assertion and added some sample test (#13) * Plotting Graphs using Chart.js (#18) * Sample data generation and table display * emptying tables * Plotting graph usingchart.js * checking file type through drag and drop and displaying alert on invalid type * removing inline function calling * little refactoring * showing checkboxes against valid columns only * Adding graphing menu for different graph types * adding colors to graph * plotting the whole data * Add files via upload * Adding CSV string import function (#28) * Adding CSV string import function * small fix * keeping up to date * Added Multiple graphs Feature (#29) * Basic Implementation of Class * Add implementation of different table generation * Added Multiple Menu For multiple graphs * Multiple Chart Generation Completed * Minor Changes * Divided into Diiferent files * Added Babel for transpiling es6 code and modularize code by movig classes to seperate files * Minor Bug Removed * Fixed Minor linting issues * Added Patch for Multiple Graphs (#36) Patch For Multiple Graph Bug * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update README.md * Update README.md * Create CONTRIBUTING.md * Create CODE_OF_CONDUCT.md (#46) * Create PULL_REQUEST_TEMPLATE.md * Update issue templates * Adding ESlint (#37) * Create Downloadable Spreadsheet using SheetJS (#43) * Changes to src * More changes * remote url access * trying to pass remote values * SheetJS Complete * Resolved Bugs * All fixes! * Removing and adding some CDNs (font awesome addition and Range slider deletion) (#52) * Remove console.log's from View.js * changed the style of the button in View.js (#54) * [IMP] Added Tests, Refactored Code, Added Plotly.js and resolved some logical bugs (#59) * Shifting parsing code from View.js ro CsvParser.js * refactoring: adding return statements * Testing with refactored code * 1st test basic * resolving constrcutor error * Changing e=import and export syntax * try * Added Tests and corrected some code * ChartJs class * Plotly Class Added * Documentation for View and Csvparser functions (#61) * Added Installation Instruction (#62) * Update README.md * Gsheet (#63) * sign-in Google * attempt for google sheets * dummy credentials * ready to host on heroku * dummy * Update package.json * Update package.json * Update package.json * Bump lodash from 4.17.11 to 4.17.15 (#67) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.15. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.11...4.17.15) Signed-off-by: dependabot[bot] <support@github.com> * Update package.json * Update package-lock.json * Implementing CODAP export (#66) * updating package.json * heroku path * . * g sheet cred * sheet Functions * v1.0.4 * v1.0.5 * prev file use * view ma * latest V * Add file Description option to save against file * ids for popover fields * v1.2.1 * assigned title and desc * CODAP export * CODAP DONE * del creds * UI testing * v 1.3.2 * removing unused files * Delete test.csv * Delete uitest.js * UI Tests (#75) * updating package.json * heroku path * . * final tests1 * view changes
IshaGupta18
added a commit
that referenced
this pull request
Aug 20, 2019
* Project Setup (#5) * PublicLab Grapher Project Setup * Added Jquery, Papaparse and chart.js dependencies in package.json (#7) * Adding the parsing feature according to new project structure * Added npm run script to watch changes and added browserify package for build (#10) * Added Mocha testing framework and chai for assertion and added some sample test (#13) * Plotting Graphs using Chart.js (#18) * Sample data generation and table display * emptying tables * Plotting graph usingchart.js * checking file type through drag and drop and displaying alert on invalid type * removing inline function calling * little refactoring * showing checkboxes against valid columns only * Adding graphing menu for different graph types * adding colors to graph * plotting the whole data * Add files via upload * Adding CSV string import function (#28) * Adding CSV string import function * small fix * keeping up to date * Added Multiple graphs Feature (#29) * Basic Implementation of Class * Add implementation of different table generation * Added Multiple Menu For multiple graphs * Multiple Chart Generation Completed * Minor Changes * Divided into Diiferent files * Added Babel for transpiling es6 code and modularize code by movig classes to seperate files * Minor Bug Removed * Fixed Minor linting issues * Added Patch for Multiple Graphs (#36) Patch For Multiple Graph Bug * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update README.md * Update README.md * Create CONTRIBUTING.md * Create CODE_OF_CONDUCT.md (#46) * Create PULL_REQUEST_TEMPLATE.md * Update issue templates * Adding ESlint (#37) * Create Downloadable Spreadsheet using SheetJS (#43) * Changes to src * More changes * remote url access * trying to pass remote values * SheetJS Complete * Resolved Bugs * All fixes! * Removing and adding some CDNs (font awesome addition and Range slider deletion) (#52) * Remove console.log's from View.js * changed the style of the button in View.js (#54) * [IMP] Added Tests, Refactored Code, Added Plotly.js and resolved some logical bugs (#59) * Shifting parsing code from View.js ro CsvParser.js * refactoring: adding return statements * Testing with refactored code * 1st test basic * resolving constrcutor error * Changing e=import and export syntax * try * Added Tests and corrected some code * ChartJs class * Plotly Class Added * Documentation for View and Csvparser functions (#61) * Added Installation Instruction (#62) * Update README.md * Gsheet (#63) * sign-in Google * attempt for google sheets * dummy credentials * ready to host on heroku * dummy * Update package.json * Update package.json * Update package.json * Bump lodash from 4.17.11 to 4.17.15 (#67) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.15. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.11...4.17.15) Signed-off-by: dependabot[bot] <support@github.com> * Update package.json * Update package-lock.json * Implementing CODAP export (#66) * updating package.json * heroku path * . * g sheet cred * sheet Functions * v1.0.4 * v1.0.5 * prev file use * view ma * latest V * Add file Description option to save against file * ids for popover fields * v1.2.1 * assigned title and desc * CODAP export * CODAP DONE * del creds * UI testing * v 1.3.2 * removing unused files * Delete test.csv * Delete uitest.js * UI Tests (#75) * updating package.json * heroku path * . * final tests1 * view changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have added eslint to the project. However, I think it needs to be configured a little more to fix the linting errors automatically. But for now, this works!
@jywarren @namangupta01 @rexagod