🔊 add logs in alibeez import employees for employees with no valid email #12
Conversation
functions/package.json
Outdated
| @@ -13,6 +13,7 @@ | |||
| "cors": "2.8.4", | |||
| "firebase-admin": "5.13.1", | |||
| "firebase-functions": "2.0.2", | |||
| "lodash.partition": "^4.6.0", | |||
There was a problem hiding this comment.
This project uses exact versions of dependencies instead of relying on semver ranges like this one. Remove the caret (^) to specify the exact version.
| employee.zenikaEmail && employee.zenikaEmail.endsWith("@zenika.com") | ||
| ); | ||
| const importRef = firebase | ||
| const[ employeesWithValidEmail, employeesWithNoValidEmail ] = partition(employees,function (o) { return o.zenikaEmail && o.zenikaEmail.endsWith('@zenika.com') }) |
There was a problem hiding this comment.
This code looks like it is not formatted like the rest of the code. Though it is not obvious, this project uses prettier to format code. Install the prettier extension for your editor and format this file to get the conforming formatting.
Additional little details:
- I invite you to use an arrow function instead of a full function as the callback to
partition - I also invite you to come up with a better name for the variable
o
| ); | ||
| const importRef = firebase | ||
| const[ employeesWithValidEmail, employeesWithNoValidEmail ] = partition(employees,function (o) { return o.zenikaEmail && o.zenikaEmail.endsWith('@zenika.com') }) | ||
| console.info('Employees with no valid email : '+ employeesWithNoValidEmail) |
There was a problem hiding this comment.
Small detail: if you look at other log messages emitted by this app, you'll see they don't use a capital letter at the start of the message. Also, there is no space before : in English.
| ); | ||
| const importRef = firebase | ||
| const[ employeesWithValidEmail, employeesWithNoValidEmail ] = partition(employees,function (o) { return o.zenikaEmail && o.zenikaEmail.endsWith('@zenika.com') }) | ||
| console.info('Employees with no valid email : '+ employeesWithNoValidEmail) |
There was a problem hiding this comment.
I'm wondering if logging one line per email would be even better. Let's discuss it.
…when an employee has no valid email
| employee => | ||
| employee.zenikaEmail && employee.zenikaEmail.endsWith("@zenika.com") | ||
| ); | ||
| employeesWithNoValidEmail.forEach(employee => { | ||
| console.info("employee with no valid email: " + employee); |
There was a problem hiding this comment.
Now that employees are logged individually, I can see more clearly that the code is printing the employees as objects directly. This can be problematic as these objects can 1) become large 2) are not very readable. What could be done instead is only printing the name of the employee.
package.json
Outdated
| @@ -14,5 +14,8 @@ | |||
| "devDependencies": { | |||
| "firebase-tools": "4.0.3", | |||
| "npm-run-all": "4.1.3" | |||
| }, | |||
| "dependencies": { | |||
| "lodash.partition": "^4.6.0" | |||
There was a problem hiding this comment.
This is not needed at the root level. Maybe you made a mistake?
|
It might be after the fight, but this might be simpler and does not require an external library : const isEmployeeMailAZenikaMail = ({ zenikaEmail }) =>
zenikaEmail && zenikaEmail.endsWith("@zenika.com");
// or
const isEmployeeMailAZenikaMail = (employee) =>
employee.zenikaEmail && employee.zenikaEmail.endsWith("@zenika.com");
const employeesWithValidEmail = employees.filter(isEmployeeMailAZenikaMail);
const employeesWithNoValidEmail = employees.filter(employee => !isEmployeeMailAZenikaMail(employee)); |
|
I think you're right, that would probably have been the best solution 💯. Dismissed it too early because of the duplicated predicate but creating a named function for it somehow slipped my mind 🙂. |
We had to switch the `noImplicitAny` TypeScript compiler option off when [upgrading to `firebase-functions@1.0.0`](65a6108#diff-dc8de0a814a0e019ed6c1deb12c75cfeR13) because it is not compatible with it (see [this issue](firebase/firebase-functions#213)). However since then `firebase-functions` has been patched, and we've upgraded so we should be able to turn the option back on. When I tried to turn it on, the compiler detected a few places where the code was relying on some legitimate implicit anys. But it did also complain about 2 other packages that were not compatible with the option, because they were lacking types. These packages are `lodash.partition` and `firebase-tools`. I chose to remove `lodash.partition` because I felt it was not pulling its weight, as suggested by @brunosabot [here](#12 (comment)). On the other hand, we really need `firebase-tools`, but this time instead of turning `noImplicitAny` off because of it, I took the time to study how it can be left on while using packages without types. It turns out it is possible to write your own type declarations for a module installed from npm, though it took a bit of trial and error to get there. I've documented what works in `@types/README.md`, and I've written a type declaration for the one and only function that we use from the package.
Add logs in the import-employees-from-alibeez function when an employee has no valid email