make leading underscores in ingest consistent#341
make leading underscores in ingest consistent#341Jason-Benson wants to merge 2 commits intoFalveyLibraryTechnology:devfrom
Conversation
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Jason-Benson. Looks like this change has caused a number of tests to fail. In some cases, this may be a sign that we need to change the tests; in other cases, it may be revealing some bad assumptions in other code that are exposed by normalizing the directory path here. Next time we get together, I should give you a tour of the test suite so you can work through fixing these things!
|
|
||
| constructor(dir: string, config: Config, queue: QueueManager) { | ||
| this.dir = dir; | ||
| this.dir = dir.endsWith("/") ? dir : dir + "/"; |
There was a problem hiding this comment.
It also occurred to me that if we wanted to trim the trailing slash rather than normalize it on, we could use a regular expression, like:
this.dir = dir.replace(/\/+$/, "");
... while I still think I like having the leading underscore, I do wonder if it's more technically correct not to have it. Maybe the right thing to do would be to fix the bug by stripping the trailing slash, and then add a feature where we can define a "job name prefix" value in the config file. That might be best of both worlds. I also suggest this in part because fewer tests may fail if we go in the opposite direction.
Anyway, just giving options for you to think about. We can discuss further!
|
@Jason-Benson, since it seems like a good time to revive this PR, here's my proposed path forward: 1.) As per my comment thread above, I think it would make sense to revise this so that we strip slashes off of paths instead of always adding slashes on. This will allow you to revert most of the changes in this PR and just focus on the assignment of the 2.) Because I like my jobs to start with underscores, I think we should add a config setting to set a job name prefix. Maybe we could add something like: ...and then in Config.ts we can add a getter method to retrieve that setting, and then we can use it directly as part of job name construction in the job here. Does that make any sense? |
TODO