search

publiclab / simple-data-grapher

Turns CSVs into graphs in a few simple steps; embeds onto other websites!
https://publiclab.github.io/simple-data-grapher/
GNU General Public License v3.0
39 stars 52 forks source link

Adding ESlint #37

Closed IshaGupta18 closed 5 years ago

IshaGupta18 commented 5 years ago

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

IshaGupta18 commented 5 years ago

@rexagod @jywarren what do you think of this one? I actually disabled some of the rules, but that's not a very good idea, I'll enable them is you think this is alright?

rexagod commented 5 years ago

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.

IshaGupta18 commented 5 years ago

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 notifications@github.com 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 https://github.com/publiclab/simple-data-grapher/pull/37?email_source=notifications&email_token=AJXHQZ7UUMVRU3QHV6XSSVLP3BRTBA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5FEFA#issuecomment-502944276, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZ4FWZAARVLQ2RAPOSTP3BRTBANCNFSM4HWIVW6Q .

rexagod commented 5 years ago

Could be. Could be inside the .eslintrc too.

IshaGupta18 commented 5 years ago

Yes, I checked, wasn't there. So this might be good to merge?

On Tue, Jun 18, 2019, 11:35 AM Pranshu Srivastava notifications@github.com 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 https://github.com/publiclab/simple-data-grapher/pull/37?email_source=notifications&email_token=AJXHQZ2B4VOLPARD3UUQB73P3B3JXA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5JCPY#issuecomment-502960447, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZZ4OP5WP6JULKWP3TLP3B3JXANCNFSM4HWIVW6Q .

rexagod commented 5 years ago

Just to confirm, you guys never really linted the code, did you?

rexagod commented 5 years ago

Because I cannot see any eslint ignore rules as of now in any of the files.

IshaGupta18 commented 5 years ago

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 notifications@github.com 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 https://github.com/publiclab/simple-data-grapher/pull/37?email_source=notifications&email_token=AJXHQZYVCNBQG3QCRWGZNPTP3B4JJA5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5JSDA#issuecomment-502962444, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZ2UGT3NC4QZBE2JRILP3B4JJANCNFSM4HWIVW6Q .

rexagod commented 5 years ago

Maybe we should wait for @namangupta01's review here, or do you believe we should proceed with the merge?

IshaGupta18 commented 5 years ago

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 notifications@github.com 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 https://github.com/publiclab/simple-data-grapher/pull/37?email_source=notifications&email_token=AJXHQZ6H5VHXBLHVWL3FMYTP3B7W5A5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5LKLA#issuecomment-502969644, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZYWYQETSMKBCN3I3LLP3B7W5ANCNFSM4HWIVW6Q .

IshaGupta18 commented 5 years ago

Thanks a lot! :D

On Tue, Jun 18, 2019, 7:42 PM Jeffrey Warren notifications@github.com wrote:

@jywarren approved this pull request.

:-) very nice!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/publiclab/simple-data-grapher/pull/37?email_source=notifications&email_token=AJXHQZ3R6NK435CS5MTNZLTP3DUL3A5CNFSM4HWIVW62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3354AQ#pullrequestreview-251125250, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZ4MD5WQYQF4ZGX5GSDP3DUL3ANCNFSM4HWIVW6Q .