GroceriStar / groceristar-fetch

json arrays for Food Tech projects
https://groceristar.github.io/groceristar-fetch/
GNU General Public License v3.0
8 stars 18 forks source link

move away basic get methods from projects. #173

Closed atherdon closed 5 years ago

atherdon commented 5 years ago

We discuss with Vadim this is before. this line is a perfect example of how our projects relate to each other code and this is not cool. in my opinion, each project should be a separated entity, without intersections. Example: we decide to update method like getGrocery for our needs at GS project. and these changes will crash our graphQL server...

170

atherdon commented 5 years ago

I separated all default methods at the top of our major project files by moving these default methods out we'll reduce the number of code and also we'll need to remove(or update) our test files, where we're calling these methods. At search project i made some changes, that will help us with this task. i created a basic __.get method, that can be used for all major our cases(i assume that it wouldn't crash, but it can)

wahaj-47 commented 5 years ago

Can't understand what's the issue here

atherdon commented 5 years ago

we have a lot of similar methods, that doing actually the same thing. I update the link at the beginning, take a look. And each project have similar methods, that just return different data, but using the same logic. I want to update and clean it up

wahaj-47 commented 5 years ago

How about just a single function which takes as a parameter the stuff to return which gets passed into parser ?

like this

const get = function(stuff) {
    return parser(stuff)
}
atherdon commented 5 years ago

did you find the __.get method?

atherdon commented 5 years ago

Ok, I'm pretty sure you know understand what should be done. buzz me with PR when it will be done

wahaj-47 commented 5 years ago

There is a get() method in search.js and there is a .get() method in lodash. Which method were you talking about ?

wahaj-47 commented 5 years ago

I think I'll need a separate file for moving these methods away, correct me if I am wrong ?

atherdon commented 5 years ago

I think the best way, for now, is to move that method into helper.js file

wahaj-47 commented 5 years ago

Would this work ?

helper.js:
const __get = function (value) {
  return parser(value);
}

chickenKyiv.js:
const getIngredients3 = __get(ingredients3)

Use:
groceristar.chickenKyiv.getIngredients3
atherdon commented 5 years ago

push your code changes to pull request. if tests will work fine - then yes. I cannot tell it by only looking on it ;)

wahaj-47 commented 5 years ago

No the test fails. Guess, I'll have to find another way.

atherdon commented 5 years ago

still open a pull request and i'll be able to help you, don't be shy with your code - commit and open pr more frequently

atherdon commented 5 years ago

i'm actually working on this module too, so can help you quickly

atherdon commented 5 years ago

245

atherdon commented 5 years ago

i made some changes at your fork. you can continue with this task

wahaj-47 commented 5 years ago

Alright

atherdon commented 5 years ago

What about search.js file. it has an old version of __get. Can you change it too?

wahaj-47 commented 5 years ago

Update it in what way ?

wahaj-47 commented 5 years ago

Again, why are we doing this, because all I've done is just create a new function that does exactly the same thing as parser() method. What is it that we are trying to achieve here ?

atherdon commented 5 years ago

just to be clear. I'm totally on your side and I agree with you. The reason why we did it in that way - because our tests were failing and i need to close this task quickly. You can feel free to go to tests, learn how they work, update method as you want - but tests should work. Did we solve it?

As you see - you looking on this problem as on the main thing. I'm looking on this problem in complex - because this module is tightly connected with about 10 different code-projects.