search

Planet-Factory / legacy-claude

The CLAuDE model
https://planet-factory.github.io/claude-docs/
MIT License
167 stars 28 forks source link

PEP8 Linting, Constant declaring & small optimizations #24

Closed WardPearce closed 2 years ago

WardPearce commented 3 years ago

Improving the readability of the code as a whole & made some basic optimizations. Using the standardized PEP8 linting guidelines (https://www.python.org/dev/peps/pep-0008/) with a 120 character line limit.

It would be worthwhile setting up a linter on your IDLE & maybe adding a github action like so https://github.com/marketplace/actions/python-linting

If I had more time I'd look into separating the code out into reusable components and possibly moving the configuration variables to a singleton.

During my time looking over the code their seemed to be a lack of DRY :(

"Slightly controversial, moved to using double quotes, but I'm not sure why..."

WardPearce commented 3 years ago

Seems to be stable still woo

ATGSilva commented 3 years ago

As discussed privately, it may be a good idea to use a 120 character limit PEP8 version to conserve vertical space. Also could do with a reduction in white-space especially around constant definitions.

WardPearce commented 3 years ago

As discussed privately, it may be a good idea to use a 120 character limit PEP8 version to conserve vertical space. Also could do with a reduction in white-space especially around constant definitions.

Possibly moving constants to a different file entirely would be worth with, Config.py or something

ATGSilva commented 3 years ago

As discussed privately, it may be a good idea to use a 120 character limit PEP8 version to conserve vertical space. Also could do with a reduction in white-space especially around constant definitions.

Possibly moving constants to a different file entirely would be worth with, Config.py or something

Entirely agreed, that way it makes things much more intuitive for people who wish to only use the code, and not contribute to development. May be worth doing this as part of this pull request.

WardPearce commented 3 years ago

As discussed privately, it may be a good idea to use a 120 character limit PEP8 version to conserve vertical space. Also could do with a reduction in white-space especially around constant definitions.

Possibly moving constants to a different file entirely would be worth with, Config.py or something

Entirely agreed, that way it makes things much more intuitive for people who wish to only use the code, and not contribute to development. May be worth doing this as part of this pull request.

Will do

WardPearce commented 3 years ago

How do we feel about moving toy_model.py into claude/__init__.py and then having the Config.py file located in claude/Config.py? Just seems wrong having everything in one folder.

WardPearce commented 3 years ago

https://github.com/Planet-Factory/claude/pull/24/commits/96f2d6af19c1f5a61148c00df559fb39d272a717

Askill commented 3 years ago

How do we feel about moving toy_model.py into claude/__init__.py and then having the Config.py file located in claude/Config.py? Just seems wrong having everything in one folder.

I think this is a problem that will solve itself eventually when the core of the model is done and we move to OO, every planet would be constructed based on a specific config file.

WardPearce commented 3 years ago

How do we feel about moving toy_model.py into claude/__init__.py and then having the Config.py file located in claude/Config.py? Just seems wrong having everything in one folder.

I think this is a problem that will solve itself eventually when the core of the model is done and we move to OO, every planet would be constructed based on a specific config file.

yeah we'll leave that for a future PR, anyways let me know if you want anything changed with this one.

ATGSilva commented 2 years ago

Stale