Move cmd out of ax-core
#615
Conversation
…05-switch-from-structopt-to-clap
…-from-structopt-to-clap
…05-switch-from-structopt-to-clap
| } | ||
|
|
||
| // NOTE(duarte): there has to be a better way of doing this | ||
| impl TryFrom<&Option<KeyPathWrapper>> for AxPrivateKey { |
There was a problem hiding this comment.
I'd rather use a custom function for this instead of TryFrom, since the implementation detail involves a side effect with a particular path default_user_identity_path.
There was a problem hiding this comment.
I agree that this implementation is not ideal but this is quite pervasive in the code base and this is already a big change 🤔
| /// Linux $XDG_CONFIG_HOME or $HOME/.config /home/alice/.config | ||
| /// macOS $HOME/Library/Application Support /Users/Alice/Library/Application Support | ||
| /// Windows {FOLDERID_RoamingAppData} C:\Users\Alice\AppData\Roaming | ||
| fn get_data_dir() -> ActyxOSResult<PathBuf> { |
There was a problem hiding this comment.
Probably better in util, but overall,
I think ax-core shouldn't care about paths at all, and this better be put in ax, consequently default loading mechanism shouldn't be written inside ax-core, instead it should be supplied by ax e.g. via dependency-injection
There was a problem hiding this comment.
I forgot to reply. I don't think addressing this right now is pertinent, though I agree. Both this and the previous issue you outlined are relevant but a bit outside the scope of this PR. Should we open an issue?
There was a problem hiding this comment.
Absolutely. There's no need to fix this in this PR
#618
I created the issue.
| use std::net::ToSocketAddrs; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct Authority { |
There was a problem hiding this comment.
Could we unify this with PortOrHostPort? Absence of the host part means a different thing depending on context (binding to INADDR_ANY or connecting to localhost, for example).
There was a problem hiding this comment.
That might be the case but I lack the knowledge to reply. We can review this together though
There was a problem hiding this comment.
Okay, reading the code more carefully we might unify it, but it doesn’t really fit very well: Authority describes how to reach an admin port, including multiaddr usage (e.g. for relaying), while PortOrHostPort describes a specific (set of) TCP/IP SocketAddr to bind a service to (i.e. multiaddr isn’t relevant).
So I’d say we leave it as is.
| path.0 | ||
| .to_str() | ||
| .and_then(|s| s.parse::<AxPrivateKey>().ok()) | ||
| .ok_or(ActyxOSError::internal("")) |
There was a problem hiding this comment.
not sure I recall where this is used, but it feels suspicious that we erase the error message and replace it by a meaningless one
Extract ax-aql and ax-types from sdk
No description provided.