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

Allow Google Sheets and Github Configuration to be passed as a list of files/directories #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

noorbuchi
Copy link
Collaborator

With these changes, you can now provide multiple values for Google Sheets and Github configurations. Here is an example of how it would work:

$ sheetshuttle run -sc file1.yaml -sc file2.yaml -gh gh_yaml.yaml -sc dir1/ -sc dir2/file3.yaml -gh gh_dir/

hello from the default plugin
sheets_keys_file: .env
sheets_config: [PosixPath('file1.yaml'), PosixPath('file2.yaml'), PosixPath('dir1'), PosixPath('dir2/file3.yaml')]
gh_config: [PosixPath('gh_yaml.yaml'), PosixPath('gh_dir')]
Additional arguments {'args': {}}

Note that the passed values are converted to Path object to facilitate checking if it exists and if it's a directory or a file.
To get the example output, few changes were added to the default plugin to print the user provided CLI arguments.

@noorbuchi noorbuchi added enhancement New feature or request in progress for tasks currently being worked on labels Nov 18, 2022
@noorbuchi noorbuchi linked an issue Nov 18, 2022 that may be closed by this pull request
@noorbuchi noorbuchi self-assigned this Nov 18, 2022
@noorbuchi
Copy link
Collaborator Author

noorbuchi commented Nov 18, 2022

Hi @jnormile @AidanNeeson ! This is a draft of the changes that allow you to take in a list of values for Google Sheets configuration as well as GitHub configuration.
As you can see in my example command, you have to include the flag before you provide each input. So here is how that would look like:

If you want to use a combination of directories and files and Sheets Config, you have to include the -sc flag before each argument.
Correct Command:

$ sheetshuttle run -sc file1.yaml -sc file2.yaml -sc dir1/ -sc dir2/file3.yaml

You cannot put the flag once.

Incorrect Command:

$ sheetshuttle run -sc file1.yaml file2.yaml dir1/ dir2/file3.yaml

Another thing to note is that there is no validation being done on the input whatsoever. The only modification is to cast the string into a Path object. So with that in mind, the list of values is passed on to the plugin with little modifications and no validations.

Keep in mind that depending on how you want to use the values in that list, you should:

  • Check if the item is a file or directory
  • Check if the item exists on the file system
  • Other checks might be needed too, but they can be done easily since each item is a Path object.

Can I get you feedback on whether this approach in taking in the CLI values is acceptable or not? I'd like to know what you think before I continue with the remaining changes. Thanks!

@jnormile
Copy link
Collaborator

@noorbuchi , this implementation seems like it would work for our project to me! @AidanNeeson , do you see any problems I might be missing?

jnormile
jnormile previously approved these changes Nov 18, 2022
Copy link
Collaborator

@jnormile jnormile left a comment

Choose a reason for hiding this comment

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

Two thumbs up from me!

AidanNeeson
AidanNeeson previously approved these changes Nov 18, 2022
Copy link
Collaborator

@AidanNeeson AidanNeeson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@noorbuchi noorbuchi dismissed stale reviews from AidanNeeson and jnormile via 9cdd6fb November 21, 2022 01:17
@noorbuchi
Copy link
Collaborator Author

The most recent changes allow the SheetCollector infrastructure to take in a list of pathlib.Path objects to use as yaml sources of configuration. Several checks are made after SheetCollector.collect_files() is called to make sure that:

  • Every path in the list exists
  • Every path in the list points to a file (not to a directory)
  • Every path in the list points to a yaml file (aka. with a .yaml or .yml suffix)

I have to do something similar for the GitHub interactions. I'm hoping to get started on that soon as well. Please let me know what you think of the current implementation to far.
If you think this PR looks good, we can go ahead and merge it as it currently stands. Then adding this feature for the GitHub side of things can be done later if it's not currently a priority. Let me know what you prefer.

@jnormile
Copy link
Collaborator

@noorbuchi Our particular implementation doesn't require any of the GitHub interactivity--it's all purely on the Google Sheets side. So no rush at all on implementing the GitHub side of things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress for tasks currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restructure how sheets config files are passed to SheetShuttle
3 participants