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

[DONOTMERGE] Simplified FauxFactory class definition. #47

Closed
wants to merge 1 commit into from

Conversation

omaciel
Copy link
Owner

@omaciel omaciel commented Sep 23, 2014

This pull requests proposes to simplify the definition of the
FauxFactory class for backwards compatibility by looking into the
module's own set of functions and individually adding them to the class
using setaatr, and removing a long list of function definitions that
were calling the newer set of functions.

def generate_html(cls, length=5):
return gen_html(length)
# Get information from ``fauxfactory`` module
for key, val in fauxfactory.__dict__.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

for key, val in dict(fauxfactory).items():?

Copy link
Owner Author

Choose a reason for hiding this comment

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

fauxfactory is a module and non-iterable.

@omaciel
Copy link
Owner Author

omaciel commented Sep 23, 2014

In [1]: from fauxfactory import FauxFactory

In [2]: FauxFactory.generate_alp
FauxFactory.generate_alpha         FauxFactory.generate_alphanumeric

In [3]: FauxFactory.generate_alpha(3)
Out[3]: u'QjP'
)$ make test-all
coverage run -m unittest discover --start-directory tests --top-level-director
y .                                                                          
..............................................................................
..............................................................................
----------------------------------------------------------------------
Ran 156 tests in 0.805s

OK

@@ -23,7 +23,7 @@ def test_not_none(self):

"""
generators = (
partial(codify, 'make-me-unicode'),
partial(FauxFactory.codify, 'make-me-unicode'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is not backward-compatible.

@Ichimonji10
Copy link
Contributor

What happens if a new gen_* method is added to the module, or a parameter to a gen_* method changes or receives a new default value? In that case, the FauxFactory class is not identical to what was formerly in place.

@omaciel
Copy link
Owner Author

omaciel commented Sep 23, 2014

@Ichimonji10

What happens if a new gen_* method is added to the module, or if default parameters change? In that case, the FauxFactory class is not identical to what was formerly in place.

Unless I misunderstood you, all functions from the module are automatically added to FauxFactory if they start with gen_, so adding new functions would still work. Now, if we ever change the arguments for our gen_ functions, then we will intentionally break backwards compatibility :)

@omaciel
Copy link
Owner Author

omaciel commented Sep 23, 2014

Using locals().items() instead of importing the module itself.

def codify(data):
# (missing-docstring) pylint:disable=C0111
return _make_unicode(data)


Copy link
Contributor

Choose a reason for hiding this comment

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

flake8 should complain about this missing line. It looks for two blank lines preceding each class or function definition.

@Ichimonji10
Copy link
Contributor

Unless I misunderstood you, all functions from the module are automatically added to FauxFactory if they start with gen_, so adding new functions would still work.

As an example, let's say a function gen_limerick is added to this method module. In that case, class method FauxFactory.gen_limerick will be created. This is not completely backward compatible.

@Ichimonji10
Copy link
Contributor

Now, if we ever change the arguments for our gen_ functions, then we will intentionally break backwards compatibility

This breakage is unnecessary. We should be able to independently vary the gen_* functions and FauxFactory.generate_* class methods, should we not? As things stand now, we can change a gen_* function and update the FauxFactory.generate_* wrapper method, and no one with old code is the wiser.

This pull requests proposes to simplify the definition of the
``FauxFactory`` class for backwards compatibility by looking into the
module's own set of functions and individually adding them to the class
using ``setaatr``, and removing a long list of function definitions that
were calling the newer set of functions.
@omaciel
Copy link
Owner Author

omaciel commented Sep 23, 2014

@Ichimonji10 are you then NACKing this PR? :)

@Ichimonji10
Copy link
Contributor

are you then NACKing this PR? :)

It's just worth weighing the costs and benefits here. We get the benefit of terse, nice-looking code, but at the cost of breaking backward compatibility in some subtle ways. Of course, you could also make the argument that that's what version numbers are for, and we're just providing a close-enough approximation of backward compatibility.

@omaciel omaciel changed the title Simplified FauxFactory class definition. [DONOTMERGE] Simplified FauxFactory class definition. Sep 23, 2014
@omaciel
Copy link
Owner Author

omaciel commented Sep 23, 2014

Getting all sorts of issues with Travis and Python3 RuntimeError: dictionary changed size during iteration. Will revisit some time later... or not.

@omaciel omaciel closed this Sep 25, 2014
@omaciel omaciel deleted the simpler-backcompat branch September 25, 2014 17:34
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.

None yet

3 participants