Skip to content
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

Add GTFS object #1

Merged
merged 9 commits into from
Dec 29, 2017
Merged

Add GTFS object #1

merged 9 commits into from
Dec 29, 2017

Conversation

LeoFrachet
Copy link
Contributor

@LeoFrachet LeoFrachet commented Dec 29, 2017

This is basically the old code used by our internal UpdateStaticData, with only some small improvements:

  • Map & Set instead of Object & Array when possible
  • code isolated
  • tests added
  • feed info as a singleton
  • items of table shapes are called shapePoint instead of shape
  • fewer dependencies: no lodash, no moment, only async & fs-extra (& acomb)
  • big cleanup and updated in the list of official GTFS keys

helpers/csv.js Outdated
return null;
}

if (string.includes('"') === false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬

const deepness = schema.deepnessByTableName[tableName];
let sampleItem;

if (deepness === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have a while loop in deepness instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it is not worth having to think about the while do or do while choice.

const rows = [];
let rowsSlice;
let position = 0;
const length = 50000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name this batchLength


if (sortedKeys.length !== arrayOfValues.length) {
if (process.notices && process.notices.addWarning) {
process.notices.addWarning(`Row not valid in table: ${JSON.stringify(item)}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you intend to make this work in a public repo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning, it will work but not sure that should be public at all?

package.json Outdated
"description": "A Node.js librairie for GTFS",
"main": "index.js",
"scripts": {
"test": "./node_modules/.bin/mocha tests.js"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm scripts already have the package context in them you can use mocha here

.gitignore Outdated
@@ -57,3 +57,5 @@ typings/
# dotenv environment variables file
.env

package-lock.json
.idea/*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.idea/

@LeoFrachet LeoFrachet merged commit c5d607a into master Dec 29, 2017
@LeoFrachet LeoFrachet deleted the feature/add-gtfs-object branch December 29, 2017 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants