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

Hotfix detecting robyn.env #292

Merged
merged 8 commits into from
Oct 19, 2022
Merged

Conversation

Shending-Help
Copy link
Contributor

Description

This PR fixes an issue with detecting robyn.env in a project, if the server is started somewhere in a nested file or the user placed robyn.env somewhere different than expected of him robyn.env won't be found and thus i added a file detector function that starts searching from an entry point where robyn server started and goes from the bottom up in the directories searching for robyn.env

@netlify
Copy link

netlify bot commented Oct 13, 2022

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit a3ee7f2
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/634f57ecb2dac10009d7e6cd

@sansyrox
Copy link
Member

sansyrox commented Oct 14, 2022

Hey @Shending-Help , Thanks for the PR. 😄

I have a suggestion regarding the implementation approach.

We should only be detecting robyn.env at the top level of the project. So, we need to search only in the parent directory of the file that is being executed instead of doing a recursive os.walk in the code.

Something similar to: os.path.dirname(os.path.get_abspath(__file__)) (I am not sure if this the correct code but something on the lines of this)

Also, the CI is failing for some reason.

@Shending-Help
Copy link
Contributor Author

@sansyrox I made it so the root of the project gets passed to the load_vars() function and pass it the directory at which Robyn gets initialized

"""Find robyn.env file in root of the project"""
if config_path is None:
config_path = Path(project_root) / "robyn.env"

"""Parse the configuration file"""
Copy link
Member

Choose a reason for hiding this comment

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

@Shending-Help , this should be the docstring of the function as this is the main purpose.

You can make the comment below as a #comment.

"""Find robyn.env file in root of the project"""

Docstrings are reflected in the function metadata and it doesn't make a lot of sense to add two of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sansyrox fixed it

@@ -26,11 +24,11 @@ def parser(config_path=CONFIG_PATH):


# check for the environment variables set in cli and if not set them
def load_vars(variables = None):
def load_vars(variables = None, root = None):
Copy link
Member

Choose a reason for hiding this comment

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

Two last nits: let's call the variable as project_root only as we want the **kwargs to be related to the function name,

Second, can you please use the pre-commit hooks like mentioned in here https://github.com/sansyrox/robyn#%EF%B8%8F-to-develop-locally

Thanks again 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sansyrox done

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @Shending-Help 🔥

@sansyrox sansyrox merged commit b4102c2 into sparckles:main Oct 19, 2022
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.

4 participants