-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Update the list of global events #625
Conversation
|
It wasn't intentional and I noticed the empty lines issue. |
I'll try to fix this later or tomorrow. |
The new line issue seems to be a bug in const data = yaml.safeLoad(contents);
const dump = yaml.safeDump(data); // => unwanted new lines. See nodeca/js-yaml#221, nodeca/js-yaml#222. Not sure what to do about it. Those issues seem to be fixed in master and it looks like that a new version of Maybe it makes sense to rebuild |
Hmm, in the meantime, how about something like |
@fhemberger I tried that, not so useful, the problem is that the major part of the new lines are added when data is dumped with We can probably work around that issue using the That's why I think it's probably better to regenerate |
08b1567
to
d4931de
Compare
I've updated the pr to work around the There is a lot of diff noise but I tried to make the review process a little "easier" by splitting the changes over multiple commits. If we wait for the next version of |
if ( | ||
title.includes('find a tech job') || | ||
title.includes('nodeschool') || | ||
/ mongodb (?:user group|meet ?up)$/.test(title) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not title.includes('mongodb')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there are some MongoDB+Node.js events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also a lot of irrelevant events like Angular Ember etc but it's kinda hard to have ONLY relevant Node.js events. This a flaw in Meetup categories and topics.
They will be updated yes, but not deleted. New events will be added. |
Great, thank you! |
This works around a js-yaml issue where spurious new lines are incorrectly added when serializing an object. See nodeca/js-yaml#222.
Some NodeSchool events don't have a `country` property, so we should check if it is defined before updating the `location` property.
d4931de
to
ba8336d
Compare
@fhemberger I've added the requested comments, let me know if they should be changed/improved. |
@lpinca LGTM! 👍 |
Follow up of #614.
This updates
events.md
to fix the Spain region issue.I've used my own API key for meetup.com and no API key for node-geocoder (it seems to work fine).