Skip to content
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

Renamed the settings file to allow users to override it #19

Merged

Conversation

jaresty
Copy link
Collaborator

@jaresty jaresty commented Feb 9, 2024

I prefer for configuration to not be committed to the repository since it makes it difficult for users to override the configuration. This change allows users to see an example that they can easily copy and paste into their own configuration folder, which will hopefully make it easier for people to use this repository without having to make their own fork.

@C-Loftus
Copy link
Owner

C-Loftus commented Feb 9, 2024

I think this can be done easier by matching on the hostname. It is easy to override this so I personally prefer having this here as a working file rather than having a dummy file. I think I'm gonna keep it as is for now

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 9, 2024

What do you mean by "matching on the hostname"? It's not intuitive to me that configuration settings would be baked into a repository. I think this will encourage forking of this repository, which I think is bad for contribution.

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 9, 2024

FWIW - I'm not opposed to having a setup script or something that copies the configuration example file to a real location in this repository that is in the .gitignore file - I've seen that work fine in other open source repositories before.

@C-Loftus
Copy link
Owner

C-Loftus commented Feb 9, 2024

You can put the hostname in the context header and it will override any settings without the context header. So the config is decoupled and easy to override. I am not sure I will change this since knausj also has settings in done in a similar way

I am a bit unsure on this so going to leave it open for time being.

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 9, 2024

I'm not a fan of this approach, but at the very least, can you put a commented out example of how to override it? I still don't know how to do this myself. If we follow the configuration practice of community, we will likely end up with the same results (i.e. many forked repositories that do not upstream contributions) over time.

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 10, 2024

I'm raising this issue on the community repo on the same topic. Thought I'd share here for visibility too: talonhub/community#1384

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 10, 2024

Regarding using the host name to override: I do commit my personal configuration to a repository and I also deploy it to multiple machines, so relying on the host name isn't convenient since it varies per computer which means I'll need a custom deployment script just for that. I haven't had a need to override any settings in community yet that were hard coded into the configuration file. Many of those have defaults that aren't committed so you can override it with a settings file anywhere in your user folder. There are a few settings that are hard coded, though, and I have struggled with how to change them since I doing want to fork the repository.

@C-Loftus
Copy link
Owner

C-Loftus commented Feb 10, 2024

OK here is what I'll say. I think having a '.example' file might not be immediately intuitive for new users. And I like having a real live talon settings file where you can edit, since the vast majority of people do not fork and the ones who do haven't really complained.
But that being said I do get where you are coming from. I think there are many scenarios where voice commands will need forks given the particularities of user set ups and or accents, but this is not one of them.

I am willing to merge this if we

  • add some documentation in the read me that explains the situation. (ie that they have to copy the example configuration, And why we are doing it this way) should be a relatively short couple bullet points under a configuration heading.
  • add a .gitignore to make sure the settings file without the example won't get committed accidentally be contributors (assuming its filename is relatively similar)
  • test to make sure everything works OK if you get rid of all settings set in talon and just fall back to the default? I think it will I just haven't tested this and what will happen if none of the settings are set in the talon file and we just fall back to the python default.

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 10, 2024

These all sound good to me. I'll put that together soon.

@jaresty
Copy link
Collaborator Author

jaresty commented Feb 10, 2024

I tested without the settings file and it does work as expected.

@jaresty jaresty force-pushed the rename-settings-to-allow-user-override branch from 14855c3 to 400a18b Compare February 10, 2024 16:34
Moved around a couple relics in the readme and made some things a bit more terse
@C-Loftus C-Loftus merged commit d292f1f into C-Loftus:main Feb 10, 2024
@C-Loftus
Copy link
Owner

Thanks, merged

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