+
Skip to content

Support source on Variable, allow more global, class level options #27

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
Merged
merged 27 commits into from
Jun 18, 2019
Merged

Support source on Variable, allow more global, class level options #27

merged 27 commits into from
Jun 18, 2019

Conversation

marcinzaremba
Copy link
Contributor

Fixes #15

@marcinzaremba marcinzaremba changed the title Support source on Variable Support source on Variable, allow more global, class level options Jun 7, 2019
thilp
thilp reviewed Jun 11, 2019
View reviewed changes
Copy link
Contributor

@thilp thilp left a comment

Choose a reason for hiding this comment

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

I like it. For curiosity, could you please explain why you decided not to go with a "data" Variable class (no behavior) but instead with a more "active" class (with Variable.get)?

@marcinzaremba @thilp
Apply suggestions from code review
709c815 
Co-Authored-By: Thibaut Le Page <thilp@thilp.net>
@marcinzaremba
Copy link
Contributor Author

I tried to factor the behavior out to the Config class but felt really bad from OOP point of view to have another class method that just solely relies on attributes from Variable and does not belong to the class per se. I also experimented to have this code directly within the main loop but then it felt load method is too heavy. It happens to be a nice compromise between design and practicality; truly pythonic, right?

@marcinzaremba
Copy link
Contributor Author

marcinzaremba commented Jun 12, 2019 •
edited
Loading

What's left here:

  • rewrite Config.load method documentation
  • describe global Config options in README.rst
  • add tests to all documented options

@marcinzaremba marcinzaremba marked this pull request as ready for review June 17, 2019 12:56
@marcinzaremba
Copy link
Contributor Author

Now waiting for @thilp final review, then apply the fixes, then I think we can release 2.0 if no major issues come up.

thilp
thilp reviewed Jun 18, 2019
View reviewed changes
marcinzaremba and others added 2 commits June 18, 2019 11:41
@marcinzaremba marcinzaremba merged commit 2f4eca1 into master Jun 18, 2019
@marcinzaremba marcinzaremba deleted the issue-15 branch June 19, 2019 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@thilp thilp thilp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

os.environb suppport?

2 participants

@marcinzaremba @thilp
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载