Save API tokens and default server URL locally #259

marcua · 2024-01-06T00:18:21Z

Resolves #251

…ult url if one isn't provided at runtime.

… for URLs and tokens

sofiaritz · 2024-01-07T12:30:08Z

src/bin/ayb.rs

+        } else {
+            home_dir()
+                .expect("can't determine home directory")
+                .join(".ayb.json")


The standard in Linux is to create config files inside $HOME/.config/$APP_NAME (or whatever $XDG_CONFIG_HOME is set to).
In this case that would be $HOME/.config/ayb/ayb.json.

The crate dirs does this for you with the dirs::config_dir function :)
There's also directories, which is higher level, and fully compliant with the standard of all the different OSes.

Thank you! I switched over to directories. Hurray for standardizing across operating systems! :)

sofiaritz · 2024-01-07T12:31:22Z

src/client/config.rs

+}
+
+impl ClientConfig {
+    fn default_config() -> ClientConfig {


As you may already know, perfect use case for Default :p

Thank you and done!

…ault_urls from .ayb.json, and test set_default_url

…tion in documentation

marcua · 2024-01-08T03:52:00Z

src/bin/ayb.rs

+            base_url: url.to_string(),
+            api_token: token,
+        };
+        if let Some(matches) = matches.subcommand_matches("create_database") {


The changes below are just unindenting by one level since we removed a layer of if to get the URL above.

marcua · 2024-01-08T03:52:27Z

src/client.rs

@@ -0,0 +1,2 @@
+pub mod config;


Starting the move toward pulling things out of http and into server and client

sofiaritz

I really like how you implemented this, great work! :)
Just some suggestions

sofiaritz · 2024-01-08T06:06:21Z

src/client/config.rs

+}
+
+impl ClientConfig {
+    pub fn new() -> ClientConfig {


Should new receive the version as a param or should we leave it as is and change it in the future if needed?

Good question! My intuition is no, since the version is a function of the schema/format of the encoded JSON, and should stay close to it. I'm adding it here now in case we ever need to change the schema/format, so that we can detect an older version of the configuration file and migrate it. The caller shouldn't have any knowledge of this internal detail of the configuration file. For example, you wouldn't want a caller to say that the configuration format is a v2 format when it is in fact v1. Let me know if that makes sense.

I think you're right, I'll mark this as resolved :)

sofiaritz · 2024-01-08T06:08:15Z

src/client/config.rs

+
+    pub fn from_file(file_path: &PathBuf) -> Result<ClientConfig, std::io::Error> {
+        if file_path.exists() {
+            let file = File::open(file_path)?;


You could use fs::read_to_string() here to simplify this function a bit

Done! I in initially had the buffered reader/writers for performance reasons, but the configuration file is so tiny that I suspect it will hardly matter :).

sofiaritz · 2024-01-08T06:09:08Z

src/client/config.rs

+            return Ok(serde_json::from_reader(&mut reader)?);
+        }
+
+        Ok(ClientConfig::new())


This should call default instead of new

sofiaritz · 2024-01-08T06:10:51Z

src/client/config.rs

+                .parent()
+                .expect("unable to determine parent of ayb configuration directory"),
+        )?;
+        let file = File::create(file_path)?;


You could use fs::write to simplify this function

Done! Thank you!

sofiaritz

I forgot one comment :p
Also, would a command like ayb client clean to delete the config file be useful?

sofiaritz · 2024-01-08T06:25:34Z

README.md

@@ -116,6 +112,13 @@ $ ayb client profile marcua
 Adam Marcus  |             |              |          | http://marcua.net 
 ```

+Note that the command line also saved a configuration file (on Linux,
+to a place like `$HOME/.config/ayb/ayb.json`) for your convenience so


Maybe adding three bullet points for Linux, MacOS and Windows would be useful for some people

… code review feedback

marcua · 2024-01-09T00:24:09Z

Thank you so much for the review @sofiaritz ! That simplified and clarified things! Let me know if my response on passing a version makes sense. Re: ayb clean, I haven't seen it in too many CLIs, but we can look out for use cases for it and add it if we encounter them. What do you think?

sofiaritz

Great implementation! I think we can leave the ayb client clean command for another day :)

The idea of the ayb client clean command is to leave the file system like it was before ayb was installed and used. That way someone who wants to remove ayb from their system can do so without leaving anything behind.

marcua · 2024-01-10T00:01:33Z

Thank you so much @sofiaritz! :)

I like the ayb client clean idea. A nice uninstaller for when you no longer want it! We can keep our eyes out for people who ask for it. Thank you again!

Save auth token to local config

c603966

marcua changed the title ~~Save auth token to local config~~ [WIP] Save auth token to local config Jan 6, 2024

marcua added 6 commits January 6, 2024 00:19

Lint

6ddb684

Missing file

de02ce0

Resolve conflicts

dbae181

Set default URL if one is provided and it's not already set. Use defa…

f5e651c

…ult url if one isn't provided at runtime.

set_default_url command

74e8b7d

Partially complete tests for relying on the client configuration file…

c269279

… for URLs and tokens

sofiaritz reviewed Jan 7, 2024

View reviewed changes

marcua added 6 commits January 7, 2024 16:08

Standardize constructor for ClientConfig, test reading tokens and def…

ee36d4e

…ault_urls from .ayb.json, and test set_default_url

Implement Default

b851c1a

Use directories crate to load standard configuration path

5b14474

Update documentation

feeb3c4

Make directory creation work, clean up some paths, and fix capitaliza…

f2cf2b8

…tion in documentation

Clippy and typo

b6dfeae

marcua changed the title ~~[WIP] Save auth token to local config~~ Save auth token to local config Jan 8, 2024

marcua commented Jan 8, 2024

View reviewed changes

Comment cleanup

b13a21c

marcua mentioned this pull request Jan 8, 2024

Bug updating profile #265

Closed

sofiaritz suggested changes Jan 8, 2024

View reviewed changes

sofiaritz reviewed Jan 8, 2024

View reviewed changes

Updated documentation, simplified file handling implementation, other…

255f7e1

… code review feedback

marcua requested a review from sofiaritz January 9, 2024 00:24

sofiaritz approved these changes Jan 9, 2024

View reviewed changes

marcua changed the title ~~Save auth token to local config~~ Save API tokens and default server URL locally Jan 9, 2024

marcua merged commit dcb7974 into main Jan 10, 2024

marcua deleted the client-save-tokens branch January 10, 2024 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save API tokens and default server URL locally #259

Save API tokens and default server URL locally #259

marcua commented Jan 6, 2024 •

edited

Loading

sofiaritz Jan 7, 2024 •

edited

Loading

marcua Jan 8, 2024

sofiaritz Jan 7, 2024

marcua Jan 8, 2024

marcua Jan 8, 2024

marcua Jan 8, 2024

sofiaritz left a comment

sofiaritz Jan 8, 2024

marcua Jan 8, 2024

sofiaritz Jan 9, 2024

sofiaritz Jan 8, 2024

marcua Jan 9, 2024

sofiaritz Jan 8, 2024

marcua Jan 9, 2024

sofiaritz Jan 8, 2024

marcua Jan 9, 2024

sofiaritz left a comment

sofiaritz Jan 8, 2024

marcua Jan 9, 2024

marcua commented Jan 9, 2024

sofiaritz left a comment

marcua commented Jan 10, 2024

Save API tokens and default server URL locally #259

Save API tokens and default server URL locally #259

Conversation

marcua commented Jan 6, 2024 • edited Loading

sofiaritz Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofiaritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofiaritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcua commented Jan 9, 2024

sofiaritz left a comment

Choose a reason for hiding this comment

marcua commented Jan 10, 2024

marcua commented Jan 6, 2024 •

edited

Loading

sofiaritz Jan 7, 2024 •

edited

Loading