nits and fixes #416
nits and fixes #416
Conversation
| private static readonly string BMX_DIR = Path.Join( | ||
| Environment.GetFolderPath( Environment.SpecialFolder.UserProfile ), |
There was a problem hiding this comment.
USER_HOME_DIR and BMX_DIR are only used inside this static class to define other paths now, so they should be private or inlined.
| @@ -1,11 +1,14 @@ | |||
| namespace D2L.Bmx; | |||
|
|
|||
| internal static class BmxPaths { | |||
| public static readonly string USER_HOME_DIR = Environment.GetFolderPath( Environment.SpecialFolder.UserProfile ); | |||
| public static readonly string BMX_DIR = Path.Join( USER_HOME_DIR, ".bmx" ); | |||
| public static readonly string CACHE_DIR = Path.Join( BMX_DIR, "\\cache" ); | |||
There was a problem hiding this comment.
the point of Path.Join is that we don't need to worry about path separators.
including a backslash also results in incorrect behaviour on POSIX systems, where backslash can be part of a valid directory name.
@alex-fang0
| File.Delete( Path.Join( BmxPaths.BMX_DIR, "awsCredsCache" ) ); | ||
| File.Delete( Path.Join( BmxPaths.BMX_DIR, "sessions" ) ); | ||
| File.Delete( Path.Join( BmxPaths.BMX_DIR, "update_check" ) ); |
There was a problem hiding this comment.
awsCredsCache and update_check are never released, so there's no need to handle them.
Moving (rather than deleting) sessions keeps user's old cached session, which is better UX.
There was a problem hiding this comment.
Sorry, what does 'released' mean here? I'm having trouble finding it on google because of the word 😅
There was a problem hiding this comment.
I mean they're not in bmx v3.0.0, so "the people" never got them.
There was a problem hiding this comment.
Ah, I see now. Thanks Chenfeng
| File.Move( BmxPaths.SESSIONS_FILE_LEGACY_NAME, BmxPaths.SESSIONS_FILE_NAME ); | ||
| } | ||
| } catch( Exception ex ) { | ||
| throw new BmxException( "Failed to initialize BMX directory (~/.bmx)", ex ); |
There was a problem hiding this comment.
Don't swallow errors on directory init, because our file logic in the rest of the codebase now assumes the necessary directories always exist. If there's an error here and is ignored, we may encounter an unexpected error down the line, which results in non-descriptive error message for the user.
This does mean that we run the slight risk of erroring out on failing to move the sessions file, when it could be ignored. But that's a much smaller concern to me. If there's an error in moving that file, there's probably something odd about the user's filesystem and may be worth investigating anyway.
| }, | ||
| System.CommandLine.Invocation.MiddlewareOrder.ExceptionHandler + 1 | ||
|
|
||
| await UpdateChecker.CheckForUpdatesAsync( configProvider.GetConfiguration() ); |
There was a problem hiding this comment.
move the update checker below directory init, so we can assume existence of standard BMX directories inside of it.
| // The default order for new middleware is after the middleware for `--help` & `--version` and can get short-circuited. | ||
| // We want our middleware (especially update checks) to almost always run, even on `--help` & `--version`, | ||
| // so we specify a custom order just after the exception handler (which is way before `--help` & `--version`). | ||
| order: MiddlewareOrder.ExceptionHandler + 1 |
There was a problem hiding this comment.
I couldn't recall exactly why we specified a middleware order here, and had to check Slack history, so it's worth explaining in a comment.
cfbao
requested review from
boarnoah,
scowing,
jharoutuniand2l,
alex-fang0 and
gord5500
No description provided.