GTFS transform #579
GTFS transform #579
Conversation
fix(user-update): fix user update error introduced in #577
Codecov Report
@@ Coverage Diff @@
## mtc-20200310 #579 +/- ##
=================================================
- Coverage 38.42% 15.67% -22.76%
=================================================
Files 315 323 +8
Lines 17255 16417 -838
Branches 5268 4991 -277
=================================================
- Hits 6630 2573 -4057
- Misses 9246 11824 +2578
- Partials 1379 2020 +641
Continue to review full report at Codecov.
|
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
This UI looks simple enough. As it is, I have just a few recommendations for change. However, a larger issue is still trying to figure where in the process of creating feed versions to transformations take place. I feel like this is an important design and implementation choice that we oughta spend some time trying to figure out so that is easily understood. Right now, the whole idea of having to select from up to 6 different retrieval methods of which I'm not sure I fully understand how they all work makes me very confused.
binh-dam-ibigroup
left a comment
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Echoing @evansiroky's comments on further discussing the workflow.
The big transformation settings file should be broken up so each component class is defined is in its own file, so things are easier to find.
binh-dam-ibigroup
left a comment
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Mostly cosmetic issues.
| </h4> | ||
| <small> | ||
| Apply this transformation to GTFS feeds created through the following methods. | ||
| </small> |
There was a problem hiding this comment.
I think it'd be good to have some documentation or a link to more documentation for users to see here so they can learn what each of these transformation "methods" are.
| /** | ||
| * Returns human readable name for transformation type. | ||
| */ | ||
| export function getTransformationName ( |
There was a problem hiding this comment.
This function seems very susceptible to funky additions and odd naming patterns for each transformation type. Already, I don't think this would work with a DeleteRecordsTransformation type. I think it'd be a little more maintainable if we used the strategy pattern and had each transformation type have it's own function to create the transformation name. Each of these functions could be composed of smaller sub-functions if needed.
There was a problem hiding this comment.
Also, it'd be great for this function to have a unit test.
lib/manager/util/transform.js
Outdated
| let name = type | ||
| // Remove transformation from name. | ||
| .replace('Transformation', '') | ||
| // Regex finds/captures words in camel case string and splits camel-cased words. | ||
| // Derived from: https://stackoverflow.com/a/18379358/915811 | ||
| .replace(/([a-z])([A-Z])/g, '$1 $2') | ||
| name = toSentenceCase(name) |
There was a problem hiding this comment.
Too fancy and complex. Also, not sure how well this would work with i18n. There's got to be a way to just use a lookup of transformation type to transformation human-readable name.
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
Build is failing due to a flow error and I have a recommendation to refactor the getTransformationName function.
Checklist
devbefore they can be merged tomaster)Description
Adds a Feed Transformation Settings item to the Feed Source Settings tab:
http://localhost:9966/feed/$feed_id/settings/transformationsThis new menu allows users to create feed transformations that correspond to changes in ibi-group/datatools-server#309. Currently, the only supported transformations (by design, until we flesh out some other use cases) are
ReplaceFileTransformationandReplaceFileFromStringTransformation.Re: #545.