search

MaG21 / curry

Currency scraper for the Dominican Republic
MIT License
19 stars 15 forks source link

Cleanup project structure #5

Closed gotjosh closed 7 years ago

gotjosh commented 7 years ago

Hey All!

Great project 👍, I'd like to help out with some of the features listed for the upcoming v2.0, took a quick look and noticed that we're using tabs vs spaces while I know this is a big discussion in some languages -- in Ruby, it's 90% of time spaces.

I also took the time to clean up a few low hanging fruits, more to come! 👌

I by no means want to start a big fuzz about the tabs vs spaces thing, if this is unwelcome please feel free to state so, it won't discourage me from contributing.

gotjosh commented 7 years ago

On the list:

I do have some questions as well:

MaG21 commented 7 years ago

Hello and thanks for taking the time to collaborate, we really appreciate it.

However due to the scope of your contribution, I'll have to take a moment to review your PR along with the suggestions you made.

Regarding Tab vs Space, I'm not a space fan but maybe we can make an exception and use spaces instead, maybe we could add some vim modeline at the beginning of source files.

more importantly, I'll have to say that this project started as something simple but then it became what it is today (I'm not proud of it). To be honest one of the main reasons we're asking for help is because we know there's some work to be done.

Any reason on why we shouldn't give this a gem-like structure? I noticed a comment that states that this was used as part of another project, so it could be easily packaged as a gem for usage - if it is a Rails app, we could add a Railtie.

I'm in love with the idea <3

Do any of the previously mentioned points do not make sense and we wouldn't want to include them?

Rubocop maybe.

gotjosh commented 7 years ago

maybe we could add some vim modeline at the beginning of source files

I think we shouldn't put our editor/IDE configuration in the source files, not everyone that contributes will use VIM

more importantly, I'll have to say that this project started as something simple but then it became what it is today (I'm not proud of it). To be honest one of the main reasons we're asking for help is because we know there's some work to be done.

The important thing here is the sharing of it with the community, despite its current state people find it useful, so it definitely gets a 👍 ❤️ from me

Regarding rubocop, I think code linting it's such an amazing thing to have, you effectively avoid silly syntax discussion on Pull Request (things like: having an extra line, wrongly indenting a method, avoiding the use of global variables, tabs vs spaces, etc) and allows us to focus on the things that really matter which is the actual logic of the feature, what's so good about it is how you can easily add new rules (they are written in Ruby) to enforce custom rules in your codebase and if you don't like a rule you can easily opt-out as well, I think we should give it a try but happy to drop the idea if you strongly oppose it.

However due to the scope of your contribution, I'll have to take a moment to review your PR along with the suggestions you made.

This pull request, does not change functionality whatsoever, the goal first is to organise before making any changes at all, you can easily view that all I have done in the first commit is change whitespace by appending ?w=1 to the commit URL which avoids showing whitespace changes and the second commit just moves classes into their own files and requires them as appropriate.

gotjosh commented 7 years ago

It's best-viewed commit by commit and with the use of ?w=1 in the URL to remove whitespace.

MaG21 commented 7 years ago

I think we shouldn't put our editor/IDE configuration in the source files, not everyone that contributes will use VIM

Indeed.

Regarding rubocop, I think code linting it's such an amazing thing to have...

I agree... totally!. But no, for now I rather have a larger unit test coverage than a stricter codebase. Yes, I know this is not a reasonable thing to say.

Replace usage of Thread.new for something more robust from ruby-concurrent

Since curry will be acting as a gem (hopefully), imposing new dependencies and technologies to users is something I would want to avoid, I'd rather stick to vanilla ruby and instead, expose mechanisms that allows users to use the technologies they want. Any thoughts on this one ?

regarding your PR, it seems ok to me, though, I'll merge your changes into a development branch that we can merge into master later.

Finally, many thanks for taking the time, we really appreciate you contribution, believe or not, we've many users already using api.marcos.do and we're hoping to increase those numbers.