URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
pvoborni commented:
"""
If it improves messages then I assume so provided that in won't be
controversial in other aspects.
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
HonzaCholasta commented:
"""
master:
* f1f63506caf88e4d86ea2bfdc7d25eceaf689bc5 Make pylint and jsl optional
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (15/03/17 05:32), Petr Vobornik wrote:
>In any case spending so much time discussing so minor change is a waste of
>time. I'd push it.
>
Will you accept patch whith improves
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
@pvoborni For the use case "easy for developers" the ```make lint``` target is
not sufficient. It tests only a small subset and doesn't check Python 3 issues.
PR #593 provides a
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
pvoborni commented:
"""
There was no result in the upstream discussion. My personal opinion is that one
way or the other can work. They are for different use cases. I tend to prefer
the "be easier for
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
HonzaCholasta commented:
"""
4.5.1 will be an official release too.
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-286727609
--
Manage your subscription for the
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (15/03/17 05:17), Jan Cholasta wrote:
>@lslebodn, nobody said that this has to be the last lint build related patch
>ever, we can change the behavior later, even on top of this
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
HonzaCholasta commented:
"""
@lslebodn, nobody said that this has to be the last lint build related patch
ever, we can change the behavior later, even on top of this PR. I would rather
push this now and
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (14/03/17 16:05), Christian Heimes wrote:
>https://github.com/freeipa/freeipa/pull/502#issue-209980292
>
>two thumbs up, one heart, no thumbs down
>
My naive assumption was
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
>PR #593 is not related to default yes; It is about something else.
Current version does not fix concerns; because default should be yes as it was
discussed in
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
>PR #593 is not related to default yes; It is about something else.
Current version does not fix concerns; because default should be yes as it was
discussed in
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
> This PR makes packaging IPA 4.5 on RHEL 7 easier for me, so thumbs up from me.
I understand it is more convenient to have less extra configure options in rhel;
But it was
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
> This PR makes packaging IPA 4.5 on RHEL 7 easier for me, so thumbs up from me.
I understand it is more convenient to have less extra configure options in rhel;
But it was
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
>PR #593 is not related to default yes; It is about something else.
Current version does not fix concerns; because default should be yes as it was
discussed in
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
PR #593 addresses @lslebodn concerns and provides a superior solution for local
pre-commit checking.
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
HonzaCholasta commented:
"""
This PR makes packaging IPA 4.5 on RHEL 7 easier for me, so thumbs up from me.
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-286650409
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
https://github.com/freeipa/freeipa/pull/502#issue-209980292
two thumbs up, one heart, no thumbs down
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (14/03/17 09:56), Christian Heimes wrote:
>The PR got three +1 / heart and not -1. I propose to get it merged for 4.5
>today.
>
I cannot see 3-times +1 in this PR.
I can see
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
The PR got three +1 / heart and not -1. I propose to get it merged for 4.5
today.
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-286487177
--
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
I am still expect some comment from @rcritten
LS
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-283938711
--
Manage your subscription
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
@tiran Needs rebase.
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-283931719
--
Manage your subscription for the Freeipa-devel mailing
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
Issues found by @HonzaCholasta were addressed and no one has raised any serious
concern that this patch should not be accepted.
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
Which audience is our primary concern here?
1. Should default settings be tailored towards downstream packager?
2. Or should defaults settings be user-friendly for upstream and
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
@tomaskrizek autoconf is a bit magic. ```--without-jslint``` is still there.
The line ```AC_ARG_WITH([jslint], ...)``` provides ```--with-jslint``` and
```--without-jslint```. But
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
Since `--without-jslint` was removed, there's actually no way to explicitly
turn off jsl (it will always be autodetected). I tried to set
`--with-jslint=no`, but that didn't
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
stlaz commented:
"""
There's an ongoing discussion about the acceptance of the patch. Removing the
ACK label until the acceptance is agreed on. Please, @lslebodn or @tomaskrizek,
add the label back once
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (01/03/17 22:37), Jan Cholasta wrote:
>I tend to agree with @lslebodn, but I don't have a strong opinion on this. I
>noticed a couple of issues though:
>
>* `--without-jslint`
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
HonzaCholasta commented:
"""
I tend to agree with @lslebodn, but I don't have a strong opinion on this. I
noticed a couple of issues though:
* `--without-jslint` does not seem to work correctly:
```
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
On (01/03/17 09:39), Petr Vobornik wrote:
>+1 Reasoning for not skipping linters was that reviewer or patch author can
>forget to run those. This problem was solved by travis
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
pvoborni commented:
"""
+1 Reasoning for not skipping linters was that reviewer or patch author can
forget to run those. This problem was solved by travis checks.
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
MartinBasti commented:
"""
Since we have gating here each PR is checked by linters, commits are checked
before pushed, that was reason why linters are optional now in build.
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
Wiki page updated (along with `--without-ipatests`` option from #364).
@lslebodn Ok, let's keep the PR open for a couple days to see if there's any
disagreement. I don't see
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
@tomaskrizek good point, I added a TODO item to the ticket,
https://pagure.io/freeipa/issue/6604#comment-415669
"""
See the full comment at
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
> But you're right we should update the wiki pages to mention the new defaults.
Such change require broader discussion.
e.g. I know that @rcritten had strong opinion about pylint
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
@lslebodn We don't want to have linters enabled by default when you run
`./configure` without options. But you're right we should update the wiki
pages to mention the new
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
It was explained on IRC
> < cheimes> lslebodn: Your proposal is missing the point of the ticket. It
> doesn't not simplify
> building, but rather improves error
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
Please add explanation to the thumb down
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-283309329
--
Manage your subscription for the
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
Writing hint together with error is the simplest solution. And still remind
developers to install pylint/jslint.
e.g.
```
diff --git a/configure.ac b/configure.ac
index
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
Please see ticket for reasoning.
My solution is the best thing, I could come up with in short time. It's not
worth the trouble to burn a lot of time on it. It's write-once code. You
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
lslebodn commented:
"""
May I know why the default was changed without design discussion?
IIRC pscacek intentionally enabled it by default. Much better approach would be
to print hint at configure time
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tiran commented:
"""
good catch, @tomaskrizek
"""
See the full comment at
https://github.com/freeipa/freeipa/pull/502#issuecomment-283290260
--
Manage your subscription for the Freeipa-devel mailing
URL: https://github.com/freeipa/freeipa/pull/502
Title: #502: Make pylint and jsl optional
tomaskrizek commented:
"""
Please clean up the confgure.ac and update freeipa.spec file as well.
```
configure.ac:375: AS_HELP_STRING([--disable-pylint],
freeipa.spec.in:16:%global
42 matches
Mail list logo
