-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
[qa] Added check migration filename script #16
Conversation
272f4b5
to
d3ec08f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress 👍
see my comments below
openwisp_utils/ci.py
Outdated
import re | ||
|
||
|
||
def CheckMigrationName(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep in mind camel case notation should be used only for classes
openwisp_utils/ci.py
Outdated
name; if default name pattern is found, exit with 1 | ||
""" | ||
MIGRATION_PATH = os.environ["MIGRATION_PATH"] | ||
PROJECT_NAME = os.environ["TRAVIS_REPO_SLUG"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is a function you don't need uppercase variable names (which is a convention to mean constants and global constants), you should use all lowercase variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, i started this task with a shell script and copied all the uppercase variables from there!
I am on fixing this! Thanks! 😄
openwisp_utils/ci.py
Outdated
|
||
# Get INGORE_SET | ||
if "openwisp-users" in PROJECT_NAME: | ||
IGNORE_SET = set(["0002_auto_20180508_2017.py"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember how many migrations contain auto
in the openwisp2 modules, this would become hard to maintain.
I think we can do something simpler which will still help us out: check only the last <n>
files (in alphabetical order) and raise an issue only if one of that file contains. The <n>
can be passed to the script as an argument and would have the default of 1
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! that sounds better than manually making the ignore list for all repositories!
However, i think instead of checking the last <n>
migrations, we can send <n>
as number of migrations to ignore.
Why? -- because if a PR has more than <n>
migration files, the prior system would fail, however, since we already know the number of existing migrations, the latter <n>
is easy to send and is independent of number of migrations in the pull request. What do you think? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, initial migrations to ignore 👍
tests/test_project/tests/test_ci.py
Outdated
environment = EnvironmentVarGuard() | ||
environment.set('MIGRATION_PATH', './tests/test_project/migrations/') | ||
open('./tests/test_project/migrations/0002_auto_20181001_0421.py', 'w').close() | ||
CASES = ['test', 'openwisp-users'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here regarding uppercase variable names
b5f5c44
to
ab863a9
Compare
I am trying it out on openwisp-users Update: While testing on openwisp-users, i realised my initial plan was not good. Update: Done, i am testing it on openwisp-users again! Update: It's working. this PR is ready for review. 😄 |
4c27c27
to
c538b7e
Compare
5ded890
to
44a40c4
Compare
openwisp_utils/ci.py
Outdated
which checking should begin, say, if checking needs to | ||
start after `0003_auto_20150410_3242.py` value will be `3` | ||
""" | ||
migrations_to_ignore = migrations_to_ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this line @atb00ker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, it was unintentional. Thanks! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atb00ker the code is good!
I don't understand this problem of not being able to run checks on our machine.
Can't we simply run the check as we currently do with isort
and flake8
?
If we have to modify every runscript.py
we have it will be painful :-(
Moreover, I like the separate script allows to do the checks in separate phases of development, usually these improvements come as last thing before committing changes, initially it's totally ok to make dirty changes to make code work :-)
The unix philosophy says "make it work first, refine and optimize once it works" :-)
Did you know that setup.py
gives two ways of registering scripts or functions that can become available from the command line? See https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html-
Check how flake8 does it:
- https://github.com/PyCQA/flake8/blob/master/setup.py
- https://github.com/PyCQA/flake8/blob/master/src/flake8/main/cli.py
In netjsonconfig in the past I used a simpler but somewhat dirtier approach (because it needs a separate executable script file):
If you could do something like this the result would be very clean and in the near future we could extract this feature and make it available as a standalone python package and promote it to a wider audience (we could write all sort of django related QA checks for example).
@nemesisdesign
After reading the comment about removing code from a repo/package, i was trying to implement the solution provided there for this problem. However, the problem with that approach is that testing will happen only when Keeping this point in mind, i changed that code.
The solution of running it like |
@atb00ker great |
f96e1e6
to
9bcdd91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, will try tomorrow.
b1e2475
to
0666d9e
Compare
@nemesisdesign Great! Thanks! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- do not raise a python exception when the shell script check fails because the python stack trace in that case is not useful information but a distraction - improved error message to handle singular and plural form, as well as separate the migrations by comma
Closes #15