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

Remove unnecessary console logging on Unit.js #709

Merged
merged 1 commit into from Sep 11, 2016

Conversation

eknkc
Copy link
Contributor

@eknkc eknkc commented Sep 10, 2016

No description provided.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Sep 11, 2016

Thanks for spotting this one.

Please do pull requests on the develop branch, not master.

Looking into this, I found some other calls to console.log in lib/function/arithmetic/xgcd.js that are commented out but still in the code. @josdejong: Should they be removed?

Here's a list for commit 1c12fb2:

@eknkc eknkc changed the base branch from master to develop September 11, 2016 09:10
@FSMaxB
Copy link
Collaborator

FSMaxB commented Sep 11, 2016

I'll merge this for now, the other console.log's can be removed later.

@FSMaxB FSMaxB merged commit b5afebd into josdejong:develop Sep 11, 2016
@ThomasBrierley
Copy link
Contributor

ThomasBrierley commented Sep 11, 2016

These could be caught by a linter, I use eslint which is highly configurable and has a "console.log" option... although I realise it might be a bit painful to start adding linting rules now.

josdejong added a commit that referenced this pull request Sep 12, 2016
@josdejong
Copy link
Owner

Thanks @eknkc for this patch.

@FSMaxB I've removed the commented console.logs. They don't do harm but it's a bit sloppy.

@ThomasBrierley we can certainly add a linter. Will be interesting to see whether we catch a lot of issues. In first instance we could run the linter as a separate script just to see what it does. Would you like to give this a try?

@josdejong
Copy link
Owner

I've just released v3.5.1 having with the logging removed.

@ThomasBrierley
Copy link
Contributor

Sure, I'll start with an eslint file and stick to style agnostic "good practice" rules for now and see how it goes. That can be run selectively and recursively within the repo by using a globally installed eslint module.

If it turns out to be useful and easy enough to conform existing code then it should be pretty easy to integrate into the gulp file / travis etc.

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

Successfully merging this pull request may close these issues.

None yet

4 participants