Skip to content

added starfix to work without default config#99

Merged
maxandersen merged 4 commits intostarfixdev:masterfrom
zedbeit:starfix-default-config
Dec 12, 2021
Merged

added starfix to work without default config#99
maxandersen merged 4 commits intostarfixdev:masterfrom
zedbeit:starfix-default-config

Conversation

@zedbeit
Copy link
Copy Markdown
Contributor

@zedbeit zedbeit commented Oct 5, 2021

Made starfix to work without config on windows and provide implementations for two IDEs, VScode and IntelliJ.


@Command(name = "clone")
public int cloneCmd(@Parameters(index = "0") String url) {
String userHome = System.getProperty("user.home"); // Get User Home Directory: /home/user_name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use getConfigFile instead of duplicating logic for lookup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to use getConfigFile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the getConfigFile creates ".config" directory, which won't be useful, that's why i didn't use it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its a mistake we have getconfigfile create it - should split it up so we have one to get the file/path without creating it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some changes. What do yo think?

Comment on lines +158 to +171
clone_path = System.getProperty("user.home") + "/code"; // set clone_path to /home/user_name/code
File configFile = getConfigFile();

try {
configFile.createNewFile();

if(isWindows()){// check if Windows OS
if(path_env.contains("Microsoft VS Code")){ // If PATH has VScode
ide = "code.cmd";
writeToYAMLFile(ide, clone_path,configFile);
} else if(path_env.contains("IntelliJ IDEA")){ // If PATH has IntelliJ
ide = "idea64.exe";
writeToYAMLFile(ide, clone_path,configFile);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have to write the default config to disk. Then it by definition is no longer a default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoided writing to disk.

// if(isMac()){}

} catch (Exception e) {
e.printStackTrace();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shuoldn't just be printing stacktrace. is this try/catch even needed?

defaultConfig();
}
} catch (Exception e) {
e.printStackTrace();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not print stakctraces raw to the user. and what do we expect that can throw an exception here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the code block might throw an exception, but I've fixed that now.


}

// Function to load default config
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does not load anything ,right ? it just setups default configuration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it just sets up default config.

}

// Function to load default config
public void defaultConfig() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this to be public.

Copy link
Copy Markdown
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general approach is sound enough - avoids storing file just to get a default config.

but a few issues around exception handling we should fix.

@maxandersen maxandersen merged commit 14d3c4d into starfixdev:master Dec 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants