These are chat archives for translate/dev

19th
Sep 2017
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:21
Hi! @unho @phlax !
phlax
@phlax
Sep 19 2017 08:21
hi
Leandro Regueiro
@unho
Sep 19 2017 08:22
Hi
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:22
I have taken a look at the review. I will be applying the comments when needed. The positional arguments can't have a required field, since they are already required. For the rest, I will answer. I may require your help for some of them, probably, don't know.
phlax
@phlax
Sep 19 2017 08:22
cool
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:23
Ok, let's start then.
@phlax , I am putting the formats.get() at the beginning of add_arguments function. Is format_dict a good name for it?
formats_dict
formats_dict = formats.get()
phlax
@phlax
Sep 19 2017 08:27
yep fine @jjmcarrascosa
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:27
or filetypes_dict
phlax
@phlax
Sep 19 2017 08:27
yep, not sure why i did it - but in most places the relationship/name is filetype
but then the model/api is format
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:28
Ok. you proposed to do the format.get() or the format.get().keys()at the beginning?
formatS*
phlax
@phlax
Sep 19 2017 08:29
if only the keys then rather make it format_names
probs more intuitive that way
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:29
ok
@phlax well you have set everywhere to leave the .keys() in the command
so to be coherent
i think it's better do only format.get()
and then in the command already pass the format_dict.keys()
phlax
@phlax
Sep 19 2017 08:33
i think only if the format_dict is used in more than one way
otherwise someone reading the code has to follow both -> dict -> list, rather than one transform at the beginning
imho
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:34
ok, as you prefer
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:57
@phlax I have checked the ones in the command script and fixed most of them. I will push -f again, ok?
phlax
@phlax
Sep 19 2017 08:57
go for it
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 08:59
tests/commands/create_project.py::test_cmd_create_project_defaults PASSED
tests/commands/create_project.py::test_cmd_create_project_title PASSED
tests/commands/create_project.py::test_cmd_create_project_checkstyle PASSED
tests/commands/create_project.py::test_cmd_create_project_filetypes PASSED
tests/commands/create_project.py::test_cmd_create_project_report_email PASSED
tests/commands/create_project.py::test_cmd_create_project_source_language PASSED
tests/commands/create_project.py::test_cmd_create_project_disabled PASSED
tests/commands/create_project.py::test_cmd_create_project_preset_mapping PASSED
tests/commands/create_project.py::test_cmd_create_project_mapping PASSED
tests/commands/create_project.py::test_cmd_create_project_fs_type PASSED
tests/commands/create_project.py::test_cmd_create_project_fs_url PASSED

====================================================== 2565 tests deselected ======================================================
=========================================== 11 passed, 2565 deselected in 9.92 seconds ============================================
@phlax done
I have marked with a "like" in the review what I have appleid
phlax
@phlax
Sep 19 2017 09:02
ah, cool
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 09:02
I didn't touch the tests ones
phlax
@phlax
Sep 19 2017 09:02
they will disappear if the code is changed
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 09:03
and some of the command script ones were still pending of confirmation by your side, so i decided also not to change them
ok
phlax
@phlax
Sep 19 2017 09:04
@unho im gonna delete some of the review comments on translate/pootle#6633 - so its easier to see what needs addressing - ok ?
Leandro Regueiro
@unho
Sep 19 2017 09:04
@phlax If it is about the "required" ones I fully agree.
phlax
@phlax
Sep 19 2017 09:04
and defaults test
Leandro Regueiro
@unho
Sep 19 2017 09:05
@phlax I agree with that one too.
@phlax If there is any other comment you want to remove please let me check first.
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 09:33
@phlax do I need to do anything?
phlax
@phlax
Sep 19 2017 09:33
not right now - im just working through bugs/feedback etc
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 09:36
ok
ping when needed
phlax
@phlax
Sep 19 2017 10:08
not quite finished @jjmcarrascosa - but i just pushed back to see if my changes broke anything else
Leandro Regueiro
@unho
Sep 19 2017 10:16
Did some additional reviews of the PR.
phlax
@phlax
Sep 19 2017 10:19
i have to afk for an hour or so - but be back soon
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 10:28
@phlax should I rebase?
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 11:02
@unho, do we have a function like validate_email that validate if a filesystem url is valid? Instead of, as you say, creating the project and then try to do new_project.config["pootle_fs.fs_url"] = options["fs_url"]that may cause an error with the project already created
Leandro Regueiro
@unho
Sep 19 2017 11:03
@jjmcarrascosa I don't know but I am pretty sure @phlax knows about it.
@jjmcarrascosa If such a function doesn't exist there probably is some code that validates it.
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 11:04
ok, great then, I will wait for him to come.
@unho I've found it
Leandro Regueiro
@unho
Sep 19 2017 11:20
Great
phlax
@phlax
Sep 19 2017 11:51
hi @jjmcarrascosa @unho no need to change anything on the fs_url validatoin
try/except is the right way i think
if you see the commit i added, i wired up the url validation
phlax
@phlax
Sep 19 2017 12:17
@unho @jjmcarrascosa i have created a breakout PR to add validation on fs_url translate/pootle#6755
Leandro Regueiro
@unho
Sep 19 2017 12:17
Great, reviewing
Leandro Regueiro
@unho
Sep 19 2017 12:29
@phlax Done reviewing translate/pootle#6755
phlax
@phlax
Sep 19 2017 12:30
cool, im adding some further validation, which will probably break more tests...
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:31
oh I see
ok
question
Anyway i need to check that
       if options["fs_type"]!="localfs" and options["fs_url"]=="":
            raise CommandError("Parameter --fs-url is mandatory "
                               "when --fs-type is not `localfs`")
phlax
@phlax
Sep 19 2017 12:32
yep
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:32
this is correct
ok
phlax
@phlax
Sep 19 2017 12:32
i just pushed to your branch
so rebase before changing anything
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:32
I have already changed anything but I will fetch and merge
phlax
@phlax
Sep 19 2017 12:32
cool
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:45
tests/commands/create_project.py::test_cmd_create_project_defaults PASSED
tests/commands/create_project.py::test_cmd_create_project_title PASSED
tests/commands/create_project.py::test_cmd_create_project_checkstyle PASSED
tests/commands/create_project.py::test_cmd_create_project_filetypes PASSED
tests/commands/create_project.py::test_cmd_create_project_report_email PASSED
tests/commands/create_project.py::test_cmd_create_project_source_language PASSED
tests/commands/create_project.py::test_cmd_create_project_disabled PASSED
tests/commands/create_project.py::test_cmd_create_project_preset_mapping PASSED
tests/commands/create_project.py::test_cmd_create_project_mapping PASSED
tests/commands/create_project.py::test_cmd_create_project_fs_type FAILED
tests/commands/create_project.py::test_cmd_create_project_fs_url PASSED

============================================================ FAILURES =============================================================
_________________________________________________ test_cmd_create_project_fs_type _________________________________________________
tests/commands/create_project.py:229: in test_cmd_create_project_fs_type
    "--fs-type=dummyfs")
../../pootle/env/local/lib/python2.7/site-packages/django/core/management/__init__.py:130: in call_command
    return command.execute(*args, **defaults)
../../pootle/env/local/lib/python2.7/site-packages/django/core/management/base.py:345: in execute
    output = self.handle(*args, **options)
../../pootle/env/local/lib/python2.7/site-packages/django/utils/decorators.py:185: in inner
    return func(*args, **kwargs)
pootle/apps/pootle_app/management/commands/create_project.py:133: in handle
    raise CommandError("Parameter --fs-url is mandatory "
E   CommandError: Parameter --fs-url is mandatory when --fs-type is not `localfs`
====================================================== 2565 tests deselected ======================================================
====================================== 1 failed, 10 passed, 2565 deselected in 9.90 seconds ======================================
phlax
@phlax
Sep 19 2017 12:46
i can adjust the test
let me get the validation stuff cleared up first...
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:46
let me push first
ok?
phlax
@phlax
Sep 19 2017 12:47
go for it
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 12:48
done
phlax
@phlax
Sep 19 2017 13:11
@unho im gonna land translate/pootle#6755
i need to look at validation on the fs_type too, but im gonna do that separately, as i fear it may break more tests
Leandro Regueiro
@unho
Sep 19 2017 13:19
k
phlax
@phlax
Sep 19 2017 13:29
@jjmcarrascosa im gonna rebase your PR on master and push, ok ?
done
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 13:37
great
so
at what stage are we
phlax
@phlax
Sep 19 2017 13:38
just doing some cleanups on tests...
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 13:42
ok, let me know
when I need to do anything
phlax
@phlax
Sep 19 2017 13:52
cool, will do - just wrangling some bugs...
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 13:57
Ok I will be afk until tomorrow
phlax
@phlax
Sep 19 2017 13:57
ok - ive got a bit more cleaning up to do
Jose J. Martinez
@jjmcarrascosa
Sep 19 2017 13:57
ok
ping you tomorrow
phlax
@phlax
Sep 19 2017 13:58
cool, laters
@unho another breakout PR translate/pootle#6757
phlax
@phlax
Sep 19 2017 14:05
also @unho im gonna land translate/pootle#6756 - ok ?
Leandro Regueiro
@unho
Sep 19 2017 14:08
@phlax translate/pootle#6757 enforces PO to be the default. I don't think we want a default format to be automatically picked.
phlax
@phlax
Sep 19 2017 14:08
not sure i follow - i dont think it does
well - in the sense that if you dont add any filetypes it does - but it always did
and i think po is a sane default
Leandro Regueiro
@unho
Sep 19 2017 14:09
Since projects can handle more than one filetype we never enforced a default one. That was to ensure that users explicitly specified the format(s) for the project.
This PR kinda defuses that.
phlax
@phlax
Sep 19 2017 14:10
in the project form i agree
but this PR doesnt change anything there
it just ensure correct order
Leandro Regueiro
@unho
Sep 19 2017 14:10
mmm
phlax
@phlax
Sep 19 2017 14:10
it always had the default
if we are gonna remove - lets do that separately, as it will most likely break something
actually - most likely break a lot of things
Leandro Regueiro
@unho
Sep 19 2017 14:11
Given that I cannot oppose your PR, despite I still don't feel it is the right behavior.
phlax
@phlax
Sep 19 2017 14:12
well, it kinda doesnt - but that is more in js
but yep, its a different issue to the one im addressing
Leandro Regueiro
@unho
Sep 19 2017 14:13
@phlax lgtmed both PRs.
phlax
@phlax
Sep 19 2017 15:10
@unho another breakout PR for fs_type validation translate/pootle#6758 . Would you look at it when you get chance, thanks
phlax
@phlax
Sep 19 2017 17:18
@jjmcarrascosa i have refactored the command a bit, and tidied up the PR - make sure you rebase before making any changes
@unho ive addresed all of the feedback i reckon on translate/pootle#6633 - can you give it a final review when you get chance
Leandro Regueiro
@unho
Sep 19 2017 17:19
Yep, in 2 mins
Leandro Regueiro
@unho
Sep 19 2017 18:42
@phlax lgtmed with nits translate/pootle#6633