-
Notifications
You must be signed in to change notification settings - Fork 4
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
Close #687, pass sources path in through the command line #755
Conversation
Conclusion: That is indeed the desired format for the command line args. |
This will have some merge conflicts with #774. I'd like to prioritize that one, so I'm holding off until that one's merged. |
Will have to update ALKilnInThePlayground.
Rationale: downloading a file is interactive. Comparing it to an existing file is observational.
612d95b
to
9663b05
Compare
I think this may be unblocked now, and ready for review. If you have bandwidth, @BryceStevenWilley, would love your input. |
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.
LGTM, tiny nit comments
lib/scope.js
Outdated
value: `Your list of files to find is empty.` | ||
value: `Your list of files is empty.` |
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.
Not sure I get this change. Why would you remove this extra information? Without context, the former report msg is clearer IMO.
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.
To get into this nit, if you're interested, this is just my gut feeling: I feel it muddies the waters because it doesn't necessarily line up with the step they wrote. Getting inaccurately specific can sometimes be more confusing than being more general. They either gave file(s) to "upload" or they gave file(s) to "compare". They didn't mention "finding" anything and they don't know what the code is doing in the background. Again, just my gut feeling. We could try asking a user, though, either now or at some future point.
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.
I get that, but I don't think this make it any clearer to the user. If I was a user, the first thing I'd think if I saw this error message, I'd think "what list?". If they only intended to pass one file to upload (even if they didn't pass any)? In their mind, they didn't pass a list, so this message is still confusing. It should be followed by a specific error message from the upload or compare steps anyway.
I guess this gets into who the messages are for. When they contain relatively little information like this, IMO they should at least be useful to developers. Several months out, the former message would be still be useful to me, whereas the latter wouldn't, and depending on how the user is communicating their issue with them, it either adds a simple search, or a lot more back and forth than necessary.
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.
Good point. What do you think of There are no file(s) to find written in your Step.
? I'll push that for now, dependent on your input. [Or maybe I'll try to think of something with less passive voice.]
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.
Your Step is missing the names of which file(s) to find.
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.
I like the Your step is missing...
one!
Edit: Blocked by #760.Will have to updateHave updated ALKilnInThePlayground.Is this format what we want?
npm run test @tag --sources=path/to/sources/dir
In this PR, I have:
Added tests for any new features or bug fixes (if relevant)Reason for this PR
This allows the caller to specify the path to the sources folder, something that's very environment specific and hard for ALKiln itself to work out. See the issue.
Links to any solved or related issues
Closes #687
Any manual testing I have done to ensure my PR is working
Tested locally with:
Tested from ALKilnInThe Playground as well.