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.

Sign up for GitHub

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

Jump to bottom

Refactoring to improved readibility #132

Merged
merged 31 commits into from
Nov 13, 2020
Merged

Refactoring to improved readibility #132

merged 31 commits into from
Nov 13, 2020

Conversation

noliveleger
Copy link
Contributor

@noliveleger noliveleger commented Nov 5, 2020 •
edited
Loading

It does not add any new features.

To do:

  • Remove Config.TRUE and Config.FALSE in favor of booleans.
  • Add all default configuration values in the dictionary template of class Config . It allows to use syntax dict[property] more often (instead of dict.get(property, default))
  • Use an helper to create framed messages (instead of creating the messages in place)
  • Use an helper to display Yes/No questions
  • Make the tests pass
  • Test upgrade from shared-database-obsolete
  • Test upgrade from master

On top of #129, close #53

@noliveleger noliveleger self-assigned this Nov 5, 2020
@noliveleger noliveleger changed the base branch from master to 53-kobo-coding-style November 5, 2020 00:52
@noliveleger noliveleger changed the title Huge refactoring to improved readibility [WIP] Huge refactoring to improved readibility Nov 5, 2020
@noliveleger noliveleger changed the title [WIP] Huge refactoring to improved readibility [WIP] Refactoring to improved readibility Nov 5, 2020
@noliveleger noliveleger requested a review from rroux November 6, 2020 16:04
@noliveleger noliveleger added enhancement New feature or request and removed in-progress labels Nov 6, 2020
@noliveleger
Copy link
Contributor Author

noliveleger commented Nov 6, 2020 •
edited by rroux
Loading

@rroux, @joshuaberetta. You are both assigned as reviewers to test this new version.
Even if it does not add any new features, there are lots of lines changed - which means bugs could (must?) have been introduced :-(.
Before releasing it, we need to be sure to test all the scenarios we are are aware of:

  • @joshuaberetta

    • Review code
    • single instance workstation
    • single instance server
    • single instance with backup
    • single instance with AWS (server or workstation)
    • single instance with AWS and backup (server or workstation)
    • front-end server
    • back-end server (primary)
    • back-end server (secondary)

  • @rroux

    • single instance workstation
    • single instance server
    • single instance with backup
    • single instance with AWS (server or workstation)
    • single instance with AWS and backup (server or workstation)
    • front-end server
    • back-end server (primary)
    • back-end server (secondary)

  • @noliveleger

    • single instance workstation
    • single instance server
    • single instance with backup
    • single instance with AWS (server or workstation)
    • single instance with AWS and backup (server or workstation)
    • front-end server
    • back-end server (primary)
    • back-end server (secondary)

@noliveleger noliveleger changed the title [WIP] Refactoring to improved readibility Refactoring to improved readibility Nov 6, 2020
@noliveleger noliveleger mentioned this pull request Nov 10, 2020
Closed
@noliveleger noliveleger removed their assignment Nov 10, 2020
joshuaberetta
joshuaberetta reviewed Nov 10, 2020
View reviewed changes
helpers/config.py Outdated Show resolved Hide resolved
@joshuaberetta
Copy link
Member

@noliveleger, as discussed, we are currently getting KeyError: 'kobodocker_path' when trying to run either ./run.py or ./run.py --setup

joshuaberetta
joshuaberetta requested changes Nov 12, 2020
View reviewed changes
helpers/cli.py Outdated Show resolved Hide resolved
helpers/command.py Outdated Show resolved Hide resolved
helpers/command.py Outdated Show resolved Hide resolved
helpers/command.py Outdated Show resolved Hide resolved
helpers/command.py Outdated Show resolved Hide resolved
helpers/template.py Outdated Show resolved Hide resolved
helpers/template.py Outdated Show resolved Hide resolved
helpers/template.py Outdated Show resolved Hide resolved
run.py
else:
if config.auto_detect_network():
Template.render(config)
Setup.update_hosts(current_config)
Setup.update_hosts(dict_)

Command.start()


if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

@noliveleger is there is reason why you decided not to go with something like argparse here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I always thought that argparse was introduced in Python3. I was wrong.
That would be a good improvement ;-) Let's open another issue for that!

tests/utils.py
Comment on lines +140 to +144
class MockUpgrading:

@staticmethod
def migrate_single_to_two_databases(config):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Was this intentionally left for later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mock class only used in tests.
The idea is just to make it pass over it without doing anything.
We don't need to upgrade the db during the test :-)
Would you prefer return instead of pass?

Copy link
Member

Choose a reason for hiding this comment

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

pass is good 👍 just checking if you wrote the method name and then forgot to complete it 😄

@noliveleger noliveleger merged commit ce9671a into 53-kobo-coding-style Nov 13, 2020
@noliveleger noliveleger deleted the refactoring branch November 13, 2020 20:37
@joshuaberetta joshuaberetta mentioned this pull request Nov 13, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuaberetta joshuaberetta joshuaberetta approved these changes

@rroux rroux Awaiting requested review from rroux

Assignees

@joshuaberetta joshuaberetta

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve coding style
2 participants
@noliveleger @joshuaberetta