1c7f556d4f
This change moves existing files, updates a few of the cross-references and paths, and fixes some formatting. It is not meant to be the final word on how the main page looks or how the other files are organized, but it gets everything roughly into shape. If the glance team wants to make changes, please do those as follow-up patches This change depends on the spec and on a feature of pbr that allows us to move where the auto-generated class reference documentation ends up in the tree. Depends-On: Ia750cb049c0f53a234ea70ce1f2bbbb7a2aa9454 Depends-On: I2bd5652bb59cbd9c939931ba2e7db1b37d2b30bb Change-Id: I9dde267793a5913acb5b1ec028cfb66bc5189783 Signed-off-by: Doug Hellmann <doug@doughellmann.com>
97 lines
4.1 KiB
ReStructuredText
97 lines
4.1 KiB
ReStructuredText
Disallowed Minor Code Changes
|
|
=============================
|
|
|
|
There are a few types of code changes that have been proposed recently that
|
|
have been rejected by the Glance team, so we want to point them out and explain
|
|
our reasoning.
|
|
|
|
If you feel an exception should be made for some particular change, please put
|
|
it on the agenda for the Glance weekly meeting so it can be discussed.
|
|
|
|
Database migration scripts
|
|
--------------------------
|
|
|
|
Once a database script has been included in a release, spelling or grammar
|
|
corrections in comments are forbidden unless you are fixing them as a part of
|
|
another stronger bug on the migration script itself. Modifying migration
|
|
scripts confuses operators and administrators -- we only want them to notice
|
|
serious problems. Their preference must take precedence over fixing spell
|
|
errors.
|
|
|
|
Typographical errors in comments
|
|
--------------------------------
|
|
|
|
Comments are not user-facing. Correcting minor misspellings or grammatical
|
|
errors only muddies the history of that part of the code, making ``git blame``
|
|
arguably less useful. So such changes are likely to be rejected. (This
|
|
prohibition, of course, does not apply to corrections of misleading or unclear
|
|
comments, or for example, an incorrect reference to a standards document.)
|
|
|
|
Misspellings in code
|
|
--------------------
|
|
|
|
Misspellings in function names are unlikely to be corrected for the "historical
|
|
clarity" reasons outlined above for comments. Plus, if a function is named
|
|
``mispelled()`` and a later developer tries to call ``misspelled()``, the
|
|
latter will result in a NameError when it's called, so the later developer will
|
|
know to use the incorrectly spelled function name.
|
|
|
|
Misspellings in variable names are more problematic, because if you have a
|
|
variable named ``mispelled`` and a later developer puts up a patch where an
|
|
updated value is assigned to ``misspelled``, Python won't complain. The "real"
|
|
variable won't be updated, and the patch won't have its intended effect.
|
|
Whether such a change is allowed will depend upon the age of the code, how
|
|
widely used the variable is, whether it's spelled correctly in other functions,
|
|
what the current test coverage is like, and so on. We tend to be very
|
|
conservative about making changes that could cause regressions. So whether a
|
|
patch that corrects the spelling of a variable name is accepted is a judgment
|
|
(or is that "judgement"?) call by reviewers. In proposing your patch, however,
|
|
be aware that your reviewers will have these concerns in mind.
|
|
|
|
Tests
|
|
-----
|
|
|
|
Occasionally someone proposes a patch that converts instances of
|
|
``assertEqual(True, whatever)`` to ``assertTrue(whatever)``, or instances of
|
|
``assertEqual(False, w)`` to ``assertFalse(w)`` in tests. Note that these are
|
|
not type safe changes and they weaken the tests. (See the Python ``unittest``
|
|
docs for details.) We tend to be very conservative about our tests and don't
|
|
like weakening changes.
|
|
|
|
We're not saying that such changes can never be made, we're just saying that
|
|
each change must be accompanied by an explanation of why the weaker test is
|
|
adequate for what's being tested.
|
|
|
|
Just to make this a bit clearer it can be shown using the following
|
|
example, comment out the lines in the runTest method alternatively::
|
|
|
|
import unittest
|
|
|
|
class MyTestCase(unittest.TestCase):
|
|
def setUp(self):
|
|
pass
|
|
|
|
class Tests(MyTestCase):
|
|
def runTest(self):
|
|
self.assertTrue('True')
|
|
self.assertTrue(True)
|
|
self.assertEqual(True, 'True')
|
|
|
|
To run this use::
|
|
|
|
python -m testtools.run test.py
|
|
|
|
Also mentioned within the unittests documentation_.
|
|
|
|
.. _documentation: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertTrue
|
|
|
|
LOG.warn to LOG.warning
|
|
-----------------------
|
|
|
|
Consistently there are proposed changes that will change all {LOG,logging}.
|
|
warn to {LOG,logging}.warning across the codebase due to the deprecation in
|
|
Python 3. While the deprecation is real, Glance uses oslo_log that provides
|
|
alias warn and solves the issue in single place for all projects using it.
|
|
These changes are not accepted due to the huge amount of refactoring they
|
|
cause for no reason.
|