-
Notifications
You must be signed in to change notification settings - Fork 0
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
feature/demrum 1571 dsym extensions #102
Conversation
src/commands/ios.ts
Outdated
} | ||
process.exit(1); |
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 was using myCommand.error('')
to end the command and exit with an error code, if you want to keep that consistent
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.
updated
const stats = statSync(absPath); | ||
if (!stats.isDirectory()) { | ||
throw new UserFriendlyError(null, `Invalid input: Expected a 'dSYMs/' directory but got a file.`); | ||
if (absPath.endsWith('.dSYM.zip') || absPath.endsWith('.dSYMs.zip')) { |
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.
The option is named --directory
, but it can take a file, too.
--path
would work better, is there any other alternative? Other commands in CLI don't use --path
option right now, they only use --directory
or --file
.
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.
Hah good point... --path sounds good to me unless we want the extra sort of runtime type check of having two different ones. I could go either way, have both --file and --directory with checks, or just --path.
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.
Just --path is going to be more simple to present in the usage message.
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.
updated
src/utils/iOSdSYMUtils.ts
Outdated
* Given a dSYMs path, process the input as per the specified format and return | ||
* the zipped files or copied files as necessary. | ||
**/ | ||
export function getZippedDSYMs(dsymsPath: string): { zipFiles: string[], uploadPath: string } { |
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'd consider adding debug logging in a function like this, to provide some level of visibility into what the tool is zipping together based on the command options.
I added logging to a similar command for sourcemaps that traverses file system and operates on some specific files under some specific conditions.
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 idea. I've added it with logger.info() at the moment which gives us at least the imports and access to the logger object to enhance it more with logger.debug() in case of errors -- it's only used in one spot but please let me know if it maps to what you had in mind.
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.
Yes, that's along the lines of what I was thinking: log the results of the directory scanning so we can clearly show what files are getting touched. Since in cases like this (and sourcemaps inject
), the user is just providing us a directory that we scan. But logs would make it very clear which specific files or folders inside of it we are actually handling.
I'd suggest logging in the next for-loop, too (line 92), for similar reasons. That way we could clearly see the other files that we found and are operating on. Not sure if logger.info() or logger.debug() is more appropriate.
Originally, I was thinking a debug log would be nice in the other if-else
branches, too. But if they are handling a single file/folder that was passed in as the --path
, then perhaps it's not needed. It's clear from the --path
what the tool will be using.
And info
level is fine too, if it's a detail that we would want to always show to the user.
src/utils/iOSdSYMUtils.ts
Outdated
* Given a dSYMs path, process the input as per the specified format and return | ||
* the zipped files or copied files as necessary. | ||
**/ | ||
export function getZippedDSYMs(dsymsPath: string): { zipFiles: string[], uploadPath: string } { |
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.
Yes, that's along the lines of what I was thinking: log the results of the directory scanning so we can clearly show what files are getting touched. Since in cases like this (and sourcemaps inject
), the user is just providing us a directory that we scan. But logs would make it very clear which specific files or folders inside of it we are actually handling.
I'd suggest logging in the next for-loop, too (line 92), for similar reasons. That way we could clearly see the other files that we found and are operating on. Not sure if logger.info() or logger.debug() is more appropriate.
Originally, I was thinking a debug log would be nice in the other if-else
branches, too. But if they are handling a single file/folder that was passed in as the --path
, then perhaps it's not needed. It's clear from the --path
what the tool will be using.
And info
level is fine too, if it's a detail that we would want to always show to the user.
.description('Upload and list zipped iOS symbolication files (dSYMs)'); | ||
.description('Upload and list iOS symbolication files (dSYMs)') | ||
.addHelpText('after', ` | ||
Examples: |
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 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.
Shall we add examples like this to subcommands themselves, too?
Would be useful for ios upload to show a zip-file and a directory example, for example
Allow the upload tool to accept more path patterns. Adding:
dSYM/
.dSYMs.zip
.dSYM.zip
Previously we only accepted dSYMs/ paths.