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

Code quality #82

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

chetverikov
Copy link

Hi @scottwrobinson !
Thanks for ODM Camo +)
I really liked the Camo and I use it in several of his projects. I wanted to add a few features in Camo, but was surprised at the lack of quality control of code...
In this PR I have added eslint config and corrected all code in accordance with it, except for tests )

P.S.: I have a few ideas for the architecture of the project. You can email me at: ma[dot]chetverikov[at]gmail[dot]com or hangouts

@chetverikov
Copy link
Author

@scottwrobinson Is camo dead?

@devdebonair
Copy link

Hope not. This library was really in to something.

@chetverikov
Copy link
Author

@devdebonair Yep. Idea is awesome. I want to help improve it but @scottwrobinson don't responding... ( May be create fork?

@jamespegg
Copy link

Hi @chetverikov ,

I think you've got good intentions, but you've changed over 5000 lines of code and 3000 of those were in a single commit. If it were me I wouldn't touch this PR because it's going to be a nightmare to work out.

If you really want to refactor the library, I would really recommend doing it in very small steps. Make a change, write a decent explanation into a PR and then ask @scottwrobinson to have a look.

Chances are the technical debt isn't high enough to warrant a full rewrite just yet, though. Adding some new functionality might go down better.

@chetverikov
Copy link
Author

@jamespegg
I think the PR with 5000 changes better than code without coherent style. When I want to add new feature I was horrified by:

  • in single file had spaces and tabs;
  • missed punctuation;
  • es6 is not used fully...
    And more.

@kurdin
Copy link

kurdin commented Sep 8, 2016

@chetverikov create a fork please and push it to npm. Let fix all those problems and push some changes, looks like @scottwrobinson wont response.

@scottwrobinson
Copy link
Owner

Hi all!

Sorry for not responding the past few months. I understand how frustrating it can be when you start using an open source project just to find out that its support is dwindling. Things got pretty busy for me over the summer and as a result Camo got pushed to the side, so I've pretty much ignored it for a bit now, as you've noticed.

First off, thanks for the PR @chetverikov! You are 100% correct that the code quality is pretty poor. I wrote the first version over a year ago and never really refactored anything so it's kind of a nightmare, as your PR reflects. I should have time to take a look at your PR tomorrow, although I'm not sure we'll merge everything all at once. Some changes may need to be incremental, but I'll have to assess that.

@kurdin, I fully respect and support your suggestion to create a fork, especially given my absence the past few months. However, I'm aiming to carve out some time in my schedule each week to support and improve Camo, so hopefully a fork isn't needed. Even better, I'm hoping to find people willing to help me maintain the repo in case I don't have time. Would love to hear from anyone that's interested!

I apologize again for not responding. And thanks again to everyone for their ideas/suggestions!

@kurdin
Copy link

kurdin commented Sep 8, 2016

@scottwrobinson You don't have to apologize because we all know what you taking about.
We all don't have enough time to do everything. It is normal. But your updates break things that were working #79 and this is not good, I guess this can be fix with more test. Camo is nice little project so let's try to keep it in good shape.

@chetverikov
Copy link
Author

Hi @scottwrobinson! It's great that you found time for the project.
I can help you with this project. I will be right to make an organization on github and move the project to /camo/camo.

@pinn3
Copy link

pinn3 commented Sep 21, 2016

I've found bits of information of the project status spread out in issues and pull requests of this repo and thought; How about creating a slack/discord/gitter/whatever channel? I want to contribute as well but all this seems a bit uncoordinated.

@chetverikov
Copy link
Author

Hi @pinn3 ) you have great ideas, but @scottwrobinson appears here once in two months )

@scottwrobinson
Copy link
Owner

Hey now @chetverikov, I can hear you 😉

Yes, @pinn3, the project is indeed uncoordinated. Just created a Slack channel for Camo. On Friday (my newly designated "Camo worktime") I'll get more set up in terms of invites, channels, objectives, etc. Since I'm a bit new to this (I may be the only developer that hasn't used Slack), suggestions are appreciated.

Currently I'm working on cherry-picking changes from this PR to merge in to Camo. While most of the changes are necessary, some can't be included because of backwards compatibility. Usually I don't like changing so much of the codebase in one release, but I do think it is necessary if we want others to be able to contribute.

Let me know if anyone wants an invite to the Slack channel. I may set up something like what socket.io has (slackin) for invites.

@chetverikov
Copy link
Author

@scottwrobinson Ooooops ) By the way, Why Slack or not Gitter ?

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.

6 participants