Skip to content

Conversation

@odesenfans
Copy link
Collaborator

Removed all dependencies to the aiohttp app object in non-API code.
This object was used all over the place to retrieve the config
object. Instead, we now use a global variable to store the config
object itself.

@odesenfans odesenfans self-assigned this Apr 7, 2022
@odesenfans
Copy link
Collaborator Author

Depends on #229.

Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

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

Great fix.
A few comments I'd like to discuss.

}


app_config = Config(schema=get_defaults())
Copy link
Member

Choose a reason for hiding this comment

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

I see multiple uses of aleph.config.app_config. Why not using aleph.config.get_config() everywhere and marking this variable as private with a _ prefix ?

Copy link
Member

Choose a reason for hiding this comment

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

Is this variable useful versus using @lru_cache on def get_config() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It conveys the intent better IMO. Using LRU cache when there's only one return value seems a bit counterintuitive.

Copy link
Member

Choose a reason for hiding this comment

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

What about the first comment ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the usage in job_utils.py is legit as we need to initialize the global variable. The other one in get_ipfs_api looks like an oversight from my part.

@odesenfans odesenfans changed the base branch from master to dev April 26, 2022 09:30
@odesenfans odesenfans force-pushed the od-remove-dependency-to-web-app-in-jobs branch from 39b60fc to 6a9fe97 Compare April 26, 2022 10:09
@odesenfans odesenfans marked this pull request as ready for review April 26, 2022 10:10
@odesenfans odesenfans force-pushed the od-remove-dependency-to-web-app-in-jobs branch from 6a9fe97 to 70b4bd2 Compare April 26, 2022 13:37
Removed all dependencies to the aiohttp app object in non-API code.
This object was used all over the place to retrieve the config
object. Instead, we now use a global variable to store the config
object itself.
@odesenfans odesenfans force-pushed the od-remove-dependency-to-web-app-in-jobs branch from 70b4bd2 to 8209122 Compare April 26, 2022 13:40
@odesenfans odesenfans merged commit 919aff2 into aleph-im:dev Apr 26, 2022
@odesenfans odesenfans deleted the od-remove-dependency-to-web-app-in-jobs branch April 26, 2022 13:48
odesenfans added a commit that referenced this pull request Apr 27, 2022
Removed all dependencies to the aiohttp app object in non-API code.
This object was used all over the place to retrieve the config
object. Instead, we now use a global variable to store the config
object itself.
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