Setup and scrape tasks which define a set of scrapers and relevant scraping options #32

esakal · 2018-03-27T06:03:26Z

Overview

Motivation

This PR was created following #28.

A task structure

A task defines a set of scrapers and relevant scraping options. A user can define multiple tasks that serve different budgets (for example separated work and home budget).

An example for a real task can be:

{
  "scrapers": [
    {
      "id": "leumi",
      "credentials": {
        "username": "",
        "password": ""
      }
    },
    {
      "id": "leumiCard",
      "credentials": {
        "username": "",
        "password": ""
      }
    }
  ],
  "options": {
    "combineInstallments": false,
    "dateDiffByMonth": 3
  },
  "output": {
    "saveLocation": "/Users/eransakal/Downloads/Transactions",
    "combineReport": true
  }
}

Setup a task

A user can manage tasks using the existing setup command npm run setup. I was working on the setup process to be as friendly as possible.

You can see the task setup menu structure here.

Changes in the repository

New dependencies

colors library was added to allow coloring feedback provided to the user while he/she setup a task. It is enriching his user experience.

Linting adjustments

Ignoring no-underscore-dangle: Since the setup process is quite complex, I split the code into multiple classes so it would be easier to read and maintain the code. The out-of-the-box rules of airbnb prevent using the convention _ to indicate a method/property should not be used as public api. I don't want to argue whether it is good or not (What rules do the community tend to override? airbnb/javascript#1089 and Why are underscore prefixes for internal methods of a React component considered bad? airbnb/javascript#1024) but until ECMA classes will provide a decent support for private methods/properties I do think this is better then not distinguishing between private and public api.
Ignoring class-methods-use-this: This one is quite annoying because it prevent you from having a public api of a class which doesn't use other inner properties/methods of the class. It forces you setting that function as static which is very intrusive and limiting the programmer freewill.

Splitting individual scrape code

A task is basically a group of individual scrapers with predefined scraping options. So basically we want to run the individual scraper multiple times when executing a task.
To keep DRY principle I needed to split the original scrape-individual.js file into two files. The first file handles the actual scraping and is used both by the individual scraper and the task scraper. The second file is the entry point when the user wants to scrape individual scraper and is used to get the relevant parameters from the user.

Extracting the report generation

A task scraper support two types of output, a single report that combine all the scrapped accounts together and for backward compatible it also allow creating multiple reports, one per each account.
For the same reason as explained above to keep the DRY principle I extracted the report generation into a separated file.

That's it, all the remaining files are there mostly to setup and scrape a task.

@eshaham Please review. Thanks!

Closes #28

This change is

…ite into helpers

eshaham

@esakal, wow, a hell of a PR.
I must say I can’t wait to be able to start using what you implemented here 👍

Other than what I wrote inline, I have several general comments:

The underscore notation of private fields makes me tick… 😄 . I would rather see functions extracted outside of the class if they are not to be shared publicly. At some point I gave up and stopped adding comments about it, but I would love to see it changed.
This was a massive PR, and I really had a hard time reviewing all of it at once. I would prefer seeing smaller incremental PRs, but anyway… Given this comment, I never tend to re-review PRs, but in this case I reserve the right to review again 😄
Would be great to see some reflection of this in the README file
This PR was created following Scraping multiple accounts together #28.

@esakal you might want to close this issue using one of the relevant keywords in the PR description.
Thanks so much for all the effort you put into this!

eshaham · 2018-03-28T04:27:56Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": 0,


@esakal hmm, I saw your comment about this in the description, and I only partially agree.
The way we could write private code in ES6 classes for now is to extract them into regular functions, outside the body of the class. I know it feels weird, but that's how I think the airbnb folks intended for it to work, and that's how I did it in the scrapers repo.

I will remove this line and will fix the code accordingly

eshaham · 2018-03-28T04:28:24Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": 0,
+    "class-methods-use-this": 0,


@esakal yeah, I agree about this one, I think it's irrelevant.

eshaham · 2018-03-28T05:58:46Z

src/helpers/files.js

  }
+  return null;


@esakal this function should probably throw an exception if the file doesn't exist

It is a good question, whether not having a file should throw an exception even if the purpose of this function is to remove that file. I think that a developer wants to delete a file, it doesn't matter for him if the file was not there to begin with as long as this file is not there once this method complete its execution.

@esakal I disagree.
Most file APIs I know, as well as OS commands, will return an error if the file you're trying to delete doesn't exist.

OK, i removed the existsAsync check condition and following your comment I assume that node fs.unlink function will result with error if the file is missing.

btw, I don't have a machine with microsoft OS to check this out, I assume node handles both OS when dealing with the file system.

eshaham · 2018-03-28T05:59:31Z

src/helpers/tasks-manager.js

+import { TASKS_FOLDER } from '../definitions';
+import { SCRAPERS } from '../helpers/scrapers';
+
+


@esakal redundant line break

eshaham · 2018-03-28T06:04:53Z

src/helpers/tasks-manager.js

+
+const taskManager = new TaskManager();
+
+export default taskManager;


@esakal we should export the class, not the instance

I'm not sure, holding this class as singleton and using the same instance can help us in the future if we will need to share things among classes. since we don't have a proper DI we can either do what I did to mimic singleton or to export the class which means that every consumer will hold its own instance.

BTW I would go with ` export { taskManager, TaskManager } which will allow the user to choose.

I don't mind changing that if you still want me to, let me know

@esakal I usually try to keep stuff simple and to avoid unnecessary "mizgun" of code.
I avoid Singletons if not needed. I wouldn't hesitate using them if I really need them, but that's not the case here.

I'm not sure what 'mizgun' means but sure, I will change it as requested

@esakal 😄
"mizgun" comes out of the Hebrew saying, "Preparing for Air condition".
The meaning for it here is, setting up stuff in preparation for a future addition, which may or may not ever come.

@eshaham It's funny I use this term a lot so I should have figure it out, I usually say הכנה למזגן so it is very close. :)

eshaham · 2018-04-01T05:53:31Z

src/setup/tasks/delete-task-handler.js

+  }
+
+  async run() {
+    if (!this._taskName) {


@esakal go back above seems to be returning an empty string, but it doesn't seem like the code handles that use case.

Nice Catch! fixed

eshaham · 2018-04-01T06:05:03Z

src/setup/tasks/modify-task-handler.js

+    return Object.keys(SCRAPERS).map((scraperId) => {
+      const result = { value: scraperId, name: SCRAPERS[scraperId].name };
+      const hasCredentials = this._taskData.scrapers
+        .findIndex(scraper => scraper.id === scraperId) !== -1;


@esakal you can just use find here

eshaham · 2018-04-01T06:08:03Z

src/setup/tasks/modify-task-handler.js

+        message: 'How many months do you want to scrape (1-12)?',
+        default: dateDiffByMonth,
+        filter: (value) => {
+          if (Number.isFinite(value) && !Number.isNaN(value)) {


@esakal does this ever happen? Doesn't input field type always returns a string?

the value is saved as a number in the json file, if you don't modify it the default value will be a number, if you type it inquirer returns it as string

eshaham · 2018-04-01T06:09:29Z

src/setup/tasks/modify-task-handler.js

+        filter: (value) => {
+          if (Number.isFinite(value) && !Number.isNaN(value)) {
+            return value;
+          } else if (typeof value === 'string' && value.match(/^[0-9]+$/)) {


@esakal wouldn't this be simpler?

try { return parseInt(value, 10); } catch (e) { console.log(...); return null; }

parseInt is not reliable for this specific task, see the following, an example taken from the link above:

var g = parseInt("He was 40") + "<br>"; console.log(g); // will print 40

eshaham · 2018-04-01T06:12:45Z

src/setup/tasks/modify-task-handler.js

+        name: 'scraperId',
+        message: 'Select a scraper',
+        when: (answers) => {
+          if (answers.action === 'delete') {


@esakal would be nice you could extract the action names into an enum

I thought about it but since they are relevant only in the scope of function I don't think it will contribute having them as a global constants. each function can declare its own actions and there is not connection between those functions

esakal · 2018-04-01T20:01:46Z

@eshaham a spoiler alert, this is going to be a bit long comment, I tried to cover everything. I will work on the inline comments soon.

I must say I can’t wait to be able to start using what you implemented here 👍

Thanks! it feels very powerful to scrape all the accounts at the same time.

The underscore notation of private fields makes me tick… 😄 . I would rather see functions extracted outside of the class if they are not to be shared publicly. At some point I gave up and stopped adding comments about it, but I would love to see it changed.

When I created the PR I was really hoping that you and I are on the same side of this debate :) I don't understand how private properties/methods were left out of the specification. It is be very hard for me to remove the _ prefix, mostly because not having a way to let other developers distinguish between private and public api is like not having an api at all. I can live with moving private functions from the class even if it breaks the class isolation/encapsulation and makes the code less readable, but not having private properties is... well... But I respect your thought and opinion and will make the relevant adjustments to ditch the _ prefix.

This was a massive PR, and I really had a hard time reviewing all of it at once. I would prefer seeing smaller incremental PRs, but anyway… Given this comment, I never tend to re-review PRs, but in this case I reserve the right to review again 😄

We are on the same page, having smaller incremental PRs are easier to follow. This is the reason I created #29 and #30 to begin with. But I don't think there is an intuitive way to split the rest of the code. This PR deals with managing & scraping tasks and splitting those two tasks is irrelevant. As a side note, I would like to recommend an awesome reviewing tool that I integrated into my company development process which was essential to the success of our project and is called reviewable.io. it is marked as beta but I think it is like that just because they forgot to remove the label. I love this tool and I think it has everything you dream for in reviewing service. Obviously you don't need to use it with the project unless you want to give it a try, but I owe it to the developers of this service to talk about it whenever I can.

Would be great to see some reflection of this in the README file

Sure, will do

@esakal you might want to close this issue using one of the relevant keywords in the PR description.

Done

Thanks so much for all the effort you put into this!

It is the minimum I can do, I really appreciate your work on those libraries and your contribution to the open banking concept in Israel.

eshaham · 2018-04-01T21:22:03Z

As a side note, I would like to recommend an awesome reviewing tool that I integrated into my company development process which was essential to the success of our project and is called reviewable.io

Could you TL;DR about what's so great about reviewable.io?
Took a peek at their website, but their explanation isn't that clear. I'm not sure what I would gain from their tool, which doesn't already exist on GitHub.

esakal · 2018-04-02T11:14:22Z

Could you TL;DR about what's so great about reviewable.io?

I think the following article layout nicely the reasons why reviewable.io transcends github reviewing support. I think it sums up to whether you believe that code reviews are essential part of the development process, if you do then once you will start using it you will not look back.

esakal

I will push my fixes in a minute

esakal · 2018-04-01T20:19:13Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": 0,


I will remove this line and will fix the code accordingly

esakal · 2018-04-01T20:22:48Z

src/helpers/files.js

  }
+  return null;


It is a good question, whether not having a file should throw an exception even if the purpose of this function is to remove that file. I think that a developer wants to delete a file, it doesn't matter for him if the file was not there to begin with as long as this file is not there once this method complete its execution.

esakal · 2018-04-01T20:23:13Z

src/helpers/tasks-manager.js

+import { TASKS_FOLDER } from '../definitions';
+import { SCRAPERS } from '../helpers/scrapers';
+
+


esakal · 2018-04-01T20:23:43Z

src/helpers/tasks-manager.js

+  async getTasksList() {
+    const files = await getFolderFiles(TASKS_FOLDER, '.json');
+    const result = files.map(file => path.basename(file, '.json'));
+    result.sort();


esakal · 2018-04-01T20:25:19Z

src/helpers/tasks-manager.js

+  }
+
+  async loadTask(taskName) {
+    if (taskName && this.isValidTaskName(taskName)) {


esakal · 2018-04-01T21:05:47Z

src/setup/tasks/delete-task-handler.js

+  }
+
+  async run() {
+    if (!this._taskName) {


Nice Catch! fixed

esakal · 2018-04-01T21:07:07Z

src/setup/tasks/modify-task-handler.js

+    return Object.keys(SCRAPERS).map((scraperId) => {
+      const result = { value: scraperId, name: SCRAPERS[scraperId].name };
+      const hasCredentials = this._taskData.scrapers
+        .findIndex(scraper => scraper.id === scraperId) !== -1;


esakal · 2018-04-01T21:08:52Z

src/setup/tasks/modify-task-handler.js

+        message: 'How many months do you want to scrape (1-12)?',
+        default: dateDiffByMonth,
+        filter: (value) => {
+          if (Number.isFinite(value) && !Number.isNaN(value)) {


the value is saved as a number in the json file, if you don't modify it the default value will be a number, if you type it inquirer returns it as string

esakal · 2018-04-01T21:10:23Z

src/setup/tasks/modify-task-handler.js

+        filter: (value) => {
+          if (Number.isFinite(value) && !Number.isNaN(value)) {
+            return value;
+          } else if (typeof value === 'string' && value.match(/^[0-9]+$/)) {


parseInt is not reliable for this specific task, see the following, an example taken from the link above:

var g = parseInt("He was 40") + "<br>"; console.log(g); // will print 40

esakal · 2018-04-01T21:12:41Z

src/setup/tasks/modify-task-handler.js

+        name: 'scraperId',
+        message: 'Select a scraper',
+        when: (answers) => {
+          if (answers.action === 'delete') {


I thought about it but since they are relevant only in the scope of function I don't think it will contribute having them as a global constants. each function can declare its own actions and there is not connection between those functions

esakal · 2018-04-02T11:31:32Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": ["error", { "allow": ["_private"] }],


Ok, I found a compromise imo.

for private methods - I moved them outside of the class if they didn't use the state of the instance , for others I removed the prefix and set @private flag of jsdoc

for private properties - I used the following strategy to provide true support for private properties. I remember that you prefer not disabling lint by row so I added the exception of _private that as you can see is private by design.

@esakal ok, better I guess.
I still need to digest this idea, but I guess it's the least bad option

sure, let me know :)

@esakal my way of saying sababa

…abels and values

esakal · 2018-04-06T21:03:15Z

@eshaham I pushed some improvements/fixes to this pr:

the task start calculation was wrong. I reduced the value by one so scraping was off by a month.
the single report csv file included all properties of transactions due to invalid usage of the json2csv api.
I improved the code in generate-report.js file as you recommended.
I added an option to ignore/exclude future transactions as part of a task.

I know this PR is already big so I try to to a few adjustments as possible. Hopefully you are experimenting reviewable.io with this branch and if this is the case so it should be much simpler to follow the adjustments.

Have a great weekend

esakal · 2018-04-08T05:40:07Z

@eshaham if you still find it difficult to review, I can split this one into two separated branches/prs:

the code that deals with scraping a task.
the code that deals with setup a task.

There are some complications with this separation:

You will lose the connection to the comments you already wrote
You will have a blocking dependency between them because there is no point adding one without the other.
There are some shared additions/changes that will be duplicated between them and will cause merge conflicts.

I know it is hard to follow big reviews when using github PR dashboard so if you think it worth the effort we can do it and tackle the complications as we go.

Please lmk if you want to continue with the PR or with the suggested alternative

eshaham · 2018-04-08T06:48:43Z

@esakal let's keep this PR, as we've already made some progress with it.
I've looked at reviewable.io, but didn't like the fact that mixing reviews in GH and reviewable will cause line comments to become top level comments, see here.
I think I will stick to GH for now.
Will go over your comments and changes and provide feedback soon.

esakal · 2018-04-08T08:10:46Z

thanks, I will wait for your feedback. Regarding reviewable.io, I also almost ruled out reviewable. But since my work project rely heavily on reviews we learned over time that once you start working with reviewable you don't feel the need to use github dashboard anymore. This is my personal experience but I understand your concerns about the comments becoming top level comments instead of inline comments.

eshaham

@esakal finally got around to review this PR, left some more comments.

eshaham · 2018-04-09T20:22:12Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": ["error", { "allow": ["_private"] }],


@esakal ok, better I guess.
I still need to digest this idea, but I guess it's the least bad option

eshaham · 2018-04-09T20:28:02Z

src/helpers/files.js

  }
+  return null;


@esakal I disagree.
Most file APIs I know, as well as OS commands, will return an error if the file you're trying to delete doesn't exist.

eshaham · 2018-04-09T20:28:19Z

src/constants.js

@@ -1,2 +1,3 @@
 const PASSWORD_FIELD = 'password';
-export default PASSWORD_FIELD;
+const DATE_AND_TIME_MOMENT_FORMAT = 'DD-MM-YYYY_HH-mm-ss';


@esakal mind changing this to DATE_TIME_FORMAT?

eshaham · 2018-04-09T20:35:25Z

src/helpers/tasks-manager.js

+
+const taskManager = new TaskManager();
+
+export default taskManager;


@esakal I usually try to keep stuff simple and to avoid unnecessary "mizgun" of code.
I avoid Singletons if not needed. I wouldn't hesitate using them if I really need them, but that's not the case here.

eshaham · 2018-04-09T20:40:41Z

src/scrape/scrape-individual.js

-    } else {
-      console.log(`error type: ${result.errorType}`);
-      console.log('error:', result.errorMessage);
+      console.log('error:', e.message);


@esakal console.error 😄

changed back to console.error

eshaham · 2018-04-09T20:51:10Z

src/setup/tasks/modify-task-handler.js

+        ]);
+
+        switch (answers.action) {
+          case 'summary':


@esakal can we use constants for the different actions?
I saw your comment about not making these global enums, but at least re-use strings under the same function.
This comment also applies to other such functions in this PR.

@eshaham I can do that. Before I change the code I would like to confirm the convention you want me to use when adding those constants at the beginning of the functions.

Do you prefer using the global constants convention like const SUMMARY_ACTION or the regular convention of variable like const summaryAction?

@esakal good question. I think uppercase SUMMARY_ACTION is better.

eshaham · 2018-04-09T20:58:26Z

src/helpers/tasks-manager.js

+    console.log(`- ${colors.bold(key)}: ${value}`);
+  }
+
+  async printTaskSummary(taskName) {


Actually I think we should consider adding it as one of the options to do on a task in the setup process, what do you think?

@esakal sounds like a good idea, but let's do that on a separate PR 😄

because there might be a feature request for printing a summary of all tasks or specific task as part of the setup process

I would still put it on a different location - either a helper procedural function. Not even sure it belongs to a Task class, as it it purely presentational.

eshaham · 2018-04-09T21:07:00Z

src/scrape/generate-reports.js

+    return acc;
+  }, []);
+  const filePath = `${saveLocation}/${moment().format('DD-MM-YYYY_HH-mm-ss')}.csv`;
+  const fileFields = ['Institude', 'Account', 'Date', 'Payee', 'Inflow', 'Installment', 'Total'];


Using Bank here instead will not fit since we scrape also credit cards companies, can you recommend other alternatives?

hmm, ok. But I would love to be consistent. I've used "bank" in the past to describe a credit card account as well. I've also used companyId. Institute is an option I guess, but we will need to fix across both repos.
Your call.

It is needed in single report since you need to associate each transaction to its scraper/account. The separated report doesn't need institute and Account but I don't mind adding them there as well for readability.

const FIELDS = ['Date', 'Payee', 'Inflow', 'Installment', 'Total']; const EXTRA_FIELDS = ['Institude', 'Account', ...FIELDS];

Wouldn't that be simpler?

eshaham · 2018-04-09T21:12:17Z

src/scrape/generate-reports.js

+  const filePath = `${saveLocation}/${moment().format('DD-MM-YYYY_HH-mm-ss')}.csv`;
+  const fileFields = ['Institude', 'Account', 'Date', 'Payee', 'Inflow', 'Installment', 'Total'];
+  const fileContent = json2csv({ data: fileTransactions, fileFields, withBOM: true });
+  await writeFile(filePath, fileContent);


@esakal I meant something extracting code to a different function in this class. But now I see it may be more complicated and the benefit is too small, so let's drop it.

eshaham · 2018-04-09T21:14:31Z

src/scrape/scrape-task.js

+      const saveLocation = `${saveLocationRootPath}/tasks/${taskName}`;
+      await generateSingleReport(scrapedAccounts, saveLocation);
+    } else {
+      const currentExecutionFolder = moment().format('DD-MM-YYYY_HH-mm-ss');


@esakal yeah, I got mixed emotions about this rule as well. Let's keep it for now though.

esakal

@eshaham I have some followup questions if you can review them.

You don't need to review the code at the moment because I'm delaying the push until all your comments will be answered. So you can ignore comments that describe changes in the code as you will not see them at the moment.

esakal · 2018-04-10T14:49:30Z

.eslintrc

@@ -3,6 +3,8 @@
    "arrow-body-style": 0,
    "no-shadow": 0,
    "no-console": 0,
+    "no-underscore-dangle": ["error", { "allow": ["_private"] }],


sure, let me know :)

esakal · 2018-04-10T14:50:35Z

src/constants.js

@@ -1,2 +1,3 @@
 const PASSWORD_FIELD = 'password';
-export default PASSWORD_FIELD;
+const DATE_AND_TIME_MOMENT_FORMAT = 'DD-MM-YYYY_HH-mm-ss';


esakal · 2018-04-10T14:54:25Z

src/helpers/files.js

  }
+  return null;


OK, i removed the existsAsync check condition and following your comment I assume that node fs.unlink function will result with error if the file is missing.

btw, I don't have a machine with microsoft OS to check this out, I assume node handles both OS when dealing with the file system.

esakal · 2018-04-10T14:55:53Z

src/helpers/tasks-manager.js

+
+const taskManager = new TaskManager();
+
+export default taskManager;


I'm not sure what 'mizgun' means but sure, I will change it as requested

esakal · 2018-04-10T14:59:57Z

src/scrape/scrape-individual.js

-    } else {
-      console.log(`error type: ${result.errorType}`);
-      console.log('error:', result.errorMessage);
+      console.log('error:', e.message);


changed back to console.error

esakal · 2018-04-10T15:01:07Z

src/setup/tasks/modify-task-handler.js

+        ]);
+
+        switch (answers.action) {
+          case 'summary':


@eshaham I can do that. Before I change the code I would like to confirm the convention you want me to use when adding those constants at the beginning of the functions.

Do you prefer using the global constants convention like const SUMMARY_ACTION or the regular convention of variable like const summaryAction?

esakal · 2018-04-11T17:16:42Z

@eshaham I handled all the comments we currently have in this PR.

I changed file tasks-manager.js into tasks.js and separated the summary creation from the task manager. I also exported the class instead of a singleton.
I'm sure you are familiar with this option but just in case, since we already have 17 commits, if you want you can always squash them into one single commit.

Night!

eshaham

@esakal looks good 😄 👍 👍

eshaham · 2018-04-15T10:07:22Z

@esakal I don't believe in squashing commits, I like to see the evolution of the PR in the commit list.

eransakal added 7 commits March 10, 2018 00:53

rebrand scraping as individual scraping and move settings read and wr…

2c42762

…ite into helpers

add task scraping (wip - without cli interactions)

4f54991

Merge remote-tracking branch 'origin/master' into tasks

f3120ea

restore content of file scrape-individual.js due to merge conflict

68c70d5

complete task scraping and setup process

1012c3d

fix verifyFolder to create nested directory in a single command

5d1c385

minor cleanups

36bf0a0

esakal mentioned this pull request Mar 27, 2018

Add transaction status eshaham/israeli-bank-scrapers#88

Merged

eshaham requested changes Apr 1, 2018

View reviewed changes

eransakal added 2 commits April 2, 2018 14:19

fix code following pr #32

ba79c91

add option to see task summary when modifying a task

c76f0da

esakal commented Apr 2, 2018

View reviewed changes

eransakal added 4 commits April 6, 2018 22:12

fix start from date calculation based on user selection

d86ff79

fix wrong fields mapping in single report

9fc2c4b

use advanced api of json2csv that allows mapping fields with custom l…

9416efa

…abels and values

allow task report creation without future transactions

ab2bebf

revert commit d86ff79 to allow scraping current month

90f9be4

eshaham requested changes Apr 9, 2018

View reviewed changes

esakal commented Apr 10, 2018

View reviewed changes

eransakal added 3 commits April 10, 2018 21:24

fix comments from review

757531e

cleanup

f7a56fa

extract values into constants

94e7fc2

eshaham approved these changes Apr 15, 2018

View reviewed changes

eshaham merged commit aba868c into eshaham:master Apr 15, 2018

esakal deleted the tasks branch May 21, 2018 20:44

		import { TASKS_FOLDER } from '../definitions';
		import { SCRAPERS } from '../helpers/scrapers';


		const taskManager = new TaskManager();

		export default taskManager;

Setup and scrape tasks which define a set of scrapers and relevant scraping options #32

Setup and scrape tasks which define a set of scrapers and relevant scraping options #32

Conversation

esakal commented Mar 27, 2018 • edited Loading

Overview

Motivation

A task structure

Setup a task

Changes in the repository

New dependencies

Linting adjustments

Splitting individual scrape code

Extracting the report generation

eshaham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esakal Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esakal commented Apr 1, 2018 • edited Loading

eshaham commented Apr 1, 2018

esakal commented Apr 2, 2018

esakal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esakal commented Apr 6, 2018

esakal commented Apr 8, 2018 • edited Loading

eshaham commented Apr 8, 2018

esakal commented Apr 8, 2018

eshaham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esakal Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esakal commented Mar 27, 2018 •

edited

Loading

eshaham left a comment •

edited

Loading

esakal Apr 10, 2018 •

edited

Loading

esakal commented Apr 1, 2018 •

edited

Loading

esakal commented Apr 8, 2018 •

edited

Loading

esakal Apr 10, 2018 •

edited

Loading

esakal left a comment •

edited

Loading

esakal Apr 10, 2018 •

edited

Loading