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

Streamline Generate #15

Merged
merged 17 commits into from
Nov 22, 2024
Merged

Conversation

claytongentry
Copy link
Member

@claytongentry claytongentry commented Nov 21, 2024

Refactors the generate interface to accept no arguments. You just call manifold generate to generate the manifolds for all workspaces in your project.

I also factored out the BigQuery service. I think we can consider BigQuery the only destination we're generating things for for the foreseeable future and skip managing the complexity of multiple targets for now.

Copied over the general generation logic from the former BigQuery service into a #generate method on Workspaces, but oop'ified it.

I also renamed the housing directory for workspaces, projects and vectors to api, reflecting the module hierarchy.

@claytongentry claytongentry marked this pull request as ready for review November 21, 2024 14:05
lib/manifold/cli.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@johncarney johncarney left a comment

Choose a reason for hiding this comment

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

Generally looks pretty good. I have a number of code-quality suggestions and there is one area of code that I think needs some deeper thought: Manifold::API::Workspace#dimensions_fields and friends.

lib/manifold/api/workspace.rb Outdated Show resolved Hide resolved
lib/manifold/api/workspace.rb Outdated Show resolved Hide resolved
lib/manifold/api/workspace.rb Outdated Show resolved Hide resolved
lib/manifold/cli.rb Outdated Show resolved Hide resolved
lib/manifold/api/project.rb Outdated Show resolved Hide resolved
lib/manifold/api/workspace.rb Outdated Show resolved Hide resolved
lib/manifold/api/workspace.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@johncarney johncarney left a comment

Choose a reason for hiding this comment

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

Looks great. I've made one further suggestion, but I'll let you decide whether to adopt it or not.

@logger.error("Vector configuration not found: #{path}")
return nil
end
raise "Vector configuration not found: #{path}" unless path.file?
Copy link
Collaborator

Choose a reason for hiding this comment

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

A non-existent or non-file file are not the only possible failure modes here. It could also be an invalid YAML file. Consider replacing this with rescues for Errno::ENOENT, Errno::EISDIR, and Psych::Exception.

@claytongentry claytongentry merged commit d232cbb into master Nov 22, 2024
3 checks passed
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