Closed Bug 1255876 Opened 8 years ago Closed 8 years ago

allow overriding of the detected file type/lexer (incorrect guessing of the .tt extension results in a diff full of red boxes)

Categories

(MozReview Graveyard :: Review Board: User Interface, defect)

Product:
MozReview Graveyard
Component:
Review Board: User Interface
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ehsan.akhgari, Assigned: salva)

References

Details

Attachments

(3 files, 3 obsolete files)

MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob
58 bytes, text/x-review-board-request
glob
: review+
Details 
pygments thinks .tt files are treetop files; the red boxes are errors.
i wonder if we can disable per-extension mappings and render this as plaintext instead.
Depends on: 1262730
this isn't something that review board extensions can alter in a supported way.

we could​ override the DiffChunkGenerator using reviewboard.diffviewer.chunk_generator.set_diff_chunk_generator_class and have that override _apply_pygments, but that sounds like a dive into pain.

a fix for this is to extend RB to allow the admin (and/or extensions?) to say "this file extension should be treated as this type of file".
Component: General → Review Board: Upstream
Summary: MozReview puts some code in red boxes → allow overriding of the detected file type/lexer (incorrect guessing of the .tt extension results in a diff full of red boxes)
Component: Review Board: Upstream → Review Board: User Interface
Assignee: nobody → salva
glob, did you talk to upstream about this?  Are they willing to take some changes?  If so, what would they look like?
Flags: needinfo?(glob)
(In reply to Mark Côté [:mcote] from comment #3)
> glob, did you talk to upstream about this?  Are they willing to take some
> changes?  If so, what would they look like?

i did, and fix they would be happy with is to extend RB to allow the admin to say "this file extension should be treated as this type of file".

to clarify further - this would be a new setting on the admin control panel that sets a pygment lexer for a given file extension.  to fix the .tt issue, we'd use that to map it to plain text.

the code the interacts with pygments is in reviewboard/reviewboard/diffviewer/chunk_generator.py (_apply_pygments).
Flags: needinfo?(glob)
It is configurable and that interfer with how we reload the plugin in ansible.

Review commit: https://reviewboard.mozilla.org/r/48397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48397/
I created this extension to allow type override. A proper way to do requires a more coupled monkey patch of Board Review core functionality. But I think this is a good first approach. Currently, it needs to be enabled manually but once we got this reviewed, I can dig into re-enabling automatic configuration.
https://reviewboard.mozilla.org/r/48397/#review45179

::: ansible/roles/docker-rbweb/files/install-reviewboard
(Diff revision 1)
> -site.run_manage_command('disable-extension',
> -                        ['pygments_override.extension.PygmentsOverride'])

Please merge this commit with the first, so initially we aren't automatically enabling.
https://reviewboard.mozilla.org/r/48393/#review45181

::: pylib/pygments_override/pygments_override/admin_urls.py:1
(Diff revision 1)
> +from __future__ import unicode_literals
> +
> +from django.conf.urls import patterns, url
> +
> +from pygments_override.extension import PygmentsOverride
> +
> +
> +urlpatterns = patterns(
> +    'pygments_override.views',
> +
> +    url(r'^$', 'configure'),
> +)

There are no views at this point, so we shouldn't have this admin_urls.py file in this commit.

::: pylib/pygments_override/pygments_override/extension.py:1
(Diff revision 1)
> +# pygments-override Extension for Review Board.

I don't think we need this.

::: pylib/pygments_override/pygments_override/extension.py:6
(Diff revision 1)
> +from django.conf.urls import patterns, include
> +from reviewboard.extensions.base import Extension

As a matter of style for this project, we tend to keep reviewboard and djblets imports in a separate group from the third party imports. So instead of the normal pep8 style, we go with
1. std lib
2. third party (not rb / djblets)
3. reviewboard and djblets
4. local application/library specific imports

so please add a blank between these.
https://reviewboard.mozilla.org/r/48395/#review45183

As glob mentioned in the bug, it's really for the best that this is solved upstream rather than in an extension maintained by us. This is definitely something we can use in the time between now and when our upstream fix is released, but we should start work on the upstream fix as well.

Based on how quickly you threw this together I anticipate you'll have no problems implementing it upstream. Please let me know if you have any questions regarding this.

::: pylib/pygments_override/pygments_override/extension.py:3
(Diff revision 1)
> -from __future__ import unicode_literals
> +from logging import info, debug
>  
> +from django.core.cache import cache
>  from django.conf import settings
>  from django.conf.urls import patterns, include
>  from reviewboard.extensions.base import Extension
>  
> +from functools import wraps
> +from os.path import splitext
> +from pygments import lexers
> +from pygments.lexers import guess_lexer_for_filename, get_lexer_by_name
> +from reviewboard.diffviewer import chunk_generator

Please sort these according to my comment[1] in the previous commit.

[1] https://reviewboard.mozilla.org/r/48393/#comment58671

::: pylib/pygments_override/pygments_override/extension.py:23
(Diff revision 1)
>          'Name': 'pygments-override',
>          'Summary': 'Customize Pygments for specific file extensions',
>      }
>  
>      def initialize(self):
> -        # Your extension initialization is done here.
> +        self.clear_cache()

I'm not really a fan of nuking the whole cache whenever we enable this - I'm guessing you're doing this to make sure old cached diffs are regenerated with different highlighting, but I don't think it's worth it overall.

::: pylib/pygments_override/pygments_override/extension.py:25
(Diff revision 1)
> +        # As we don't know the order of importing, let's patch both so in case
> +        # chunk_generator module did not import guess_lexer_for_filename yet,
> +        # first line will make chunk_generator import it later while the second
> +        # line will patch the current version in case the original version was
> +        # already imported.
> +        lexers.guess_lexer_for_filename = get_lexer
> +        chunk_generator.guess_lexer_for_filename = get_lexer
> +        info('guess_lexer_for_filename() monkeypatch done!')

If we're going to do this in an extension, I'd much prefer we substitute our own `DiffChunkGenerator` and override `DiffChunkGenerator._apply_pygments`. 

We can call `reviewboard.diffviewer.chunk_generator.set_diff_chunk_generator_class` to instruct Review Board to use ours. For a similar example where we override the `DiffOpcodeGenerator` see `mozreview.diffviewer.opcode_generator.NoFilterDiffOpcodeGenerator` and where we make use of it.

::: pylib/pygments_override/pygments_override/extension.py:50
(Diff revision 1)
> +    preferred = _get_preferred_lexer(fp)
> +    debug('preferred lexer %s', preferred)
> +    return preferred or guess_lexer_for_filename(fp, *args, **kwargs)
> +
> +def _get_preferred_lexer(fp):
> +    _, ext = splitext(fp)

`ext = os.path.basename(fp)`

::: pylib/pygments_override/pygments_override/extension.py:59
(Diff revision 1)
> +def _get_extension_to_type_map():
> +    return _parse_extension_overrides(_get_overrides())
> +

I think this should be rolled into a single function, especially since `_parse_extension_overrides` can be a single line.

::: pylib/pygments_override/pygments_override/extension.py:63
(Diff revision 1)
> +    pairs = lambda overrides : map(lambda override : override.split('='), overrides.split())
> +    return dict([(ext, lexername) for ext, lexername in pairs(overrides)])

`return dict([override.split('=') for override in overrides.split()])` is equivalent. Although, I think I'd prefer comma separated rather than whitespace.

::: pylib/pygments_override/pygments_override/extension.py:69
(Diff revision 1)
> +def _with_prefix(f):
> +    @wraps(f)
> +    def decorator(msg, *args):
> +        return f('[pygments_override] %s' % msg, *args)
> +    return decorator
> +
> +info = _with_prefix(info)
> +debug = _with_prefix(debug)

Please replace this with something like (which you can stick above the class / function definitions:
```python
logger = logging.getLogger(__name__)

# Then to Make a call to info
logger.info('information here')
```

It will take care of prepending the module information to the logs.
https://reviewboard.mozilla.org/r/48399/#review45229

::: pylib/pygments_override/pygments_override/admin_urls.py
(Diff revision 1)
> -from __future__ import unicode_literals

huh, the urlpatterns is thrown off by unicode literals? that's surprising to me... What happened when you had unicode literals here?

::: pylib/pygments_override/pygments_override/extension.py:81
(Diff revision 1)
> -        return f('[pygments_override] %s' % msg, *args)
> +        global _id
> +        _id += 1

Why did you feel the need for this log id? The logs should appear in order?

::: pylib/pygments_override/pygments_override/forms.py:6
(Diff revision 1)
> +    overrides = forms.CharField(
> +        required=False,
> +        help_text=u'Use a white space separated list of .ext=type pairs. I.e: .tt=json'
> +    )

It's probably worth specifying a textarea[1] for the widget here, I think it will have a much better experience as the list grows.

I guess whitespace delimited can be nicer in a text area too since you could do one per line, feel free to keep it whitespace rather than commas.

[1] https://docs.djangoproject.com/en/1.9/ref/forms/widgets/#textarea
https://reviewboard.mozilla.org/r/48395/#review45183

Of coruse. I'm glad to do it. Actually, doing in the upstream means we can tie it to the repository and to the global configuration (with a "more specific wins" rule). Even more, this serve as a test for the ultimate solution.

> I'm not really a fan of nuking the whole cache whenever we enable this - I'm guessing you're doing this to make sure old cached diffs are regenerated with different highlighting, but I don't think it's worth it overall.

Yup. Neither I am and it's OK for me to remove this. More over because you won't appreciate the changes until you wipe out you http cache manually as the responses are heavily cached. When done in the upstream, the Etag calculation and cache key will have the lexer included so they will change when their associated lexer changes.

> If we're going to do this in an extension, I'd much prefer we substitute our own `DiffChunkGenerator` and override `DiffChunkGenerator._apply_pygments`. 
> 
> We can call `reviewboard.diffviewer.chunk_generator.set_diff_chunk_generator_class` to instruct Review Board to use ours. For a similar example where we override the `DiffOpcodeGenerator` see `mozreview.diffviewer.opcode_generator.NoFilterDiffOpcodeGenerator` and where we make use of it.

I did not realize about that `set_diff_chink_generator_class`. Let's take a look.

> `ext = os.path.basename(fp)`

The [basename](https://docs.python.org/2.6/library/os.path.html#os.path.basename) is the name of the file + extension. I only need the extension.

> `return dict([override.split('=') for override in overrides.split()])` is equivalent. Although, I think I'd prefer comma separated rather than whitespace.

Well, I'll do that way and remove one level of stacke by putting all together inside `_get_extension_to_type_map()`. This split helped me while I was developing the extension as I could return intermediate results, implementing more and more as needed.

> Please replace this with something like (which you can stick above the class / function definitions:
> ```python
> logger = logging.getLogger(__name__)
> 
> # Then to Make a call to info
> logger.info('information here')
> ```
> 
> It will take care of prepending the module information to the logs.

Oh, really? Great!
https://reviewboard.mozilla.org/r/48399/#review45229

> huh, the urlpatterns is thrown off by unicode literals? that's surprising to me... What happened when you had unicode literals here?

Fortunately, nothing was happening here. But I removed it because it was introduced by the boilerplate generator and we are using Python 2.6 --so, no `__future__` at all, ;)

> Why did you feel the need for this log id? The logs should appear in order?

Yes, but **duplicated**! It was really annoying.

> It's probably worth specifying a textarea[1] for the widget here, I think it will have a much better experience as the list grows.
> 
> I guess whitespace delimited can be nicer in a text area too since you could do one per line, feel free to keep it whitespace rather than commas.
> 
> [1] https://docs.djangoproject.com/en/1.9/ref/forms/widgets/#textarea

I like the idea and I feel a little bit dumb for not coming up with it myself.
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/1-2/
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/1-2/
Comment on attachment 8744197 [details]
MozReview Request: Bug 1255876 - Disabling automatic activation of this plugin; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48397/diff/1-2/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/1-2/
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/2-3/
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/2-3/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/2-3/
Attachment #8744197 - Attachment is obsolete: true
Attachment #8744197 - Flags: review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/3-4/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/3-4/
https://reviewboard.mozilla.org/r/48395/#review45183

> Oh, really? Great!

Unfortunately, reviewboard log is not showing this info.
:smacleod as I'm not used to work with hg I'm not really sure about I did this right. I was trying to solve the issues commit per commit. I saw reviewboard tracks the changes from revision to revision and I think I was able to make that way for almost every comment you left me but for the refactor I preferred to push a complete new commit.

Once, reviewed I can redo the hostory according to your advice.

What do you think?
Flags: needinfo?(smacleod)
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

https://reviewboard.mozilla.org/r/48393/#review45531

::: pylib/pygments_override/pygments_override/extension.py:1
(Diff revision 3)
> +from django.conf import settings
> +from django.conf.urls import patterns, include

We should remove these unused imports.
Attachment #8744195 - Flags: review+
(In reply to Salvador de la Puente González [:salva] from comment #28)
> :smacleod as I'm not used to work with hg I'm not really sure about I did
> this right. I was trying to solve the issues commit per commit. I saw
> reviewboard tracks the changes from revision to revision and I think I was
> able to make that way for almost every comment you left me but for the
> refactor I preferred to push a complete new commit.

I'm curious to know what makes pushing a complete new commit your preference?
Is it just the mechanics of doing it using mercurial? In any case, it's not a
big deal for this change.

Don't hesitate to ping me on irc if you have questions about hg or how to
accomplish certain tasks with it.

> Once, reviewed I can redo the hostory according to your advice.
> 
> What do you think?

Cool, sounds like a plan.
Flags: needinfo?(smacleod)
https://reviewboard.mozilla.org/r/48395/#review45183

> The [basename](https://docs.python.org/2.6/library/os.path.html#os.path.basename) is the name of the file + extension. I only need the extension.

Ah yup, you're totally correct, I think I was confusing mention of "split" in the documentation with "splitext". In that case could you make this `ext = os.path.basename(fp)[1]`, or use a different variable name then `_`. Generally we reserve `_` for imports of `ugettext`.

> Unfortunately, reviewboard log is not showing this info.

That's suprising to me - which log file are you checking, and what do you see there?
https://reviewboard.mozilla.org/r/48717/#review45547

::: pylib/pygments_override/pygments_override/extension.py:2
(Diff revision 1)
> -from logging import info, debug
> -
> +import logging
> +from functools import wraps

This import is unused now.

::: pylib/pygments_override/pygments_override/extension.py:6
(Diff revision 1)
> +from reviewboard.diffviewer.chunk_generator import (get_diff_chunk_generator_class,
> +                                                    set_diff_chunk_generator_class)

Please use the following import format for mutiline imports:
```
from reviewboard.diffviewer.chunk_generator import (
    get_diff_chunk_generator_class,
    set_diff_chunk_generator_class,
)
```

(Note: This style choice is different than the code style Review Board itself uses, which is the style we usually follow. The style you used initialliy is what should be used for contributions to upstream Review Board).

::: pylib/pygments_override/pygments_override/extension.py:10
(Diff revision 1)
> -from os.path import splitext
> -from pygments import lexers
> -from pygments.lexers import guess_lexer_for_filename, get_lexer_by_name
> +                                                           set_overrides)
> +
> +logger = logging.getLogger(__name__)

Two blanks.

::: pylib/pygments_override/pygments_override/extension.py:12
(Diff revision 1)
> -from pygments.lexers import guess_lexer_for_filename, get_lexer_by_name
> +logger = logging.getLogger(__name__)
> -from reviewboard.diffviewer import chunk_generator
>  

Two blank lines between globally defined fucntions and variables.

::: pylib/pygments_override/pygments_override/extension.py:27
(Diff revision 1)
>      }
>  
>      is_configurable = True
>  
>      def initialize(self):
> -        self.clear_cache()
> +        overrides_map = self._get_overrides_map()

Rather than fetching and storing these values, we should access them directly from settings every time we need them. When you update the settings through Review Board's admin panel it has code which will automatically synchronize the new settings to every web head, and future access will return the new values. This would allow us to update the settings at runtime without re-enabling the extension.

You can even have the settings access take place directly in the `OverrideableDiffChunkGenerator`. In order to get a reference to the instantiated `PygmentsOverride` `Extension` object you first need to fetch the extension manager, and it will return you the extension. e.g.:
```Python
ext = get_extension_manager().get_enabled_extension(
    'pygments_override.extension.PygmentsOverride')
```
We have examples where we do this currently[1] in an extension (which will show you the import paths).

After you have the extension object you can access `ext.settings` for the up to date values.

[1] https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0a2fb6bd2b358add301a76d47fe2dc22bcc426bf/pylib/mozreview/mozreview/autoland/resources.py#163

::: pylib/pygments_override/pygments_override/overridable_chunk_generator.py:14
(Diff revision 1)
> +DiffChunkGenerator = get_diff_chunk_generator_class()
> +
> +class OverridableDiffChunkGenerator(DiffChunkGenerator):

Two blanks (There are other cases in this file where the globals should have two blanks between their definitions, but some of them are going to dissapear due to my other issues, so I'll just mark this one).

::: pylib/pygments_override/pygments_override/overridable_chunk_generator.py:17
(Diff revision 1)
> +    '''Modified DiffChunkGenerator to obey per extension overrides for syntax
> +    highlighting.
> +    '''

Please use `"""` as the docstring delimiters, rather than `'''`.

Also, the summary should always fit on a single line, I'd suggest: `A chunk generator which overrides syntax highlighting`.

You can always expand with additional paragraphs below if you'd like to add details about where the overrides come from and how the work.

::: pylib/pygments_override/pygments_override/overridable_chunk_generator.py:22
(Diff revision 1)
> +        '''Applies Pygments syntax-highlighting to a file's contents attending
> +        to per extension overrides.
> +        The resulting HTML will be returned as a list of lines.
> +        '''

Keep the generic summary (so it fits on a single line) and put the details about extension overrides blow it.

Also `"""` for docstring

::: pylib/pygments_override/pygments_override/overridable_chunk_generator.py:43
(Diff revision 1)
> +
> +    def _get_preferred_lexer(self, filename):
> +        _, ext = splitext(filename)
> +        lexername = get_overrides().get(ext, None)
> +        try:
> +            lexer = get_lexer_by_name(lexername)

We should probably pass the same options to `get_lexer_by_name` that Review Board passes to `guess_lexer_for_filename` so they get passed on to the lexer (that is, `stripnl=False` and `encoding='utf-8'`).
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

https://reviewboard.mozilla.org/r/48399/#review45559

There are a few issues I didn't mark here because the code is removed in the next commit.

::: pylib/pygments_override/pygments_override/admin_urls.py:1
(Diff revision 4)
> +

no blank here

::: pylib/pygments_override/pygments_override/admin_urls.py:5
(Diff revision 4)
> +from pygments_override.forms import PygmentsOverrideSettingsForm
> +
> +urlpatterns = patterns('',

two blanks.

::: pylib/pygments_override/pygments_override/forms.py:3
(Diff revision 4)
> +from djblets.extensions.forms import SettingsForm
> +
> +class PygmentsOverrideSettingsForm(SettingsForm):

two blanks.
Attachment #8744198 - Flags: review+
https://reviewboard.mozilla.org/r/48399/#review45229

> Yes, but **duplicated**! It was really annoying.

That's really strange...

> I like the idea and I feel a little bit dumb for not coming up with it myself.

You thought of whitespace delimited, I thought of textarea, yay for code review :D
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

https://reviewboard.mozilla.org/r/48399/#review45747

::: pylib/pygments_override/pygments_override/admin_urls.py:8
(Diff revision 4)
> +    url(r'^$', 'reviewboard.extensions.views.configure_extension', {
> +        'ext_class': PygmentsOverride,
> +        'form_class': PygmentsOverrideSettingsForm
> +    }),

this indentation triggers PEP8 E128: continuation line under-indented for visual indent
Attachment #8744198 - Flags: review?(glob)
https://reviewboard.mozilla.org/r/48399/#review45753

::: pylib/pygments_override/pygments_override/forms.py:10
(Diff revision 4)
> +class PygmentsOverrideSettingsForm(SettingsForm):
> +    overrides = forms.CharField(
> +        widget=forms.Textarea,
> +        required=False,
> +        help_text=u'Use a list of .ext=type pairs, one per line. I.e: .tt=json'
> +    )

need to validate the data provided text is well formed.

a clean_overrides() method should be triggered by django prior to it being saved - throw a ValidationError when required.
Comment on attachment 8744895 [details]
MozReview Request: Bug 1255876 - Refactored to set a custom diff chunk generator instead of monkeypatching pygments; r=glob

https://reviewboard.mozilla.org/r/48717/#review45749

::: pylib/pygments_override/pygments_override/extension.py:28
(Diff revision 1)
>  
>      is_configurable = True
>  
>      def initialize(self):
> -        self.clear_cache()
> -
> +        overrides_map = self._get_overrides_map()
> +        self._original_diff_chunk_generator_class = get_diff_chunk_generator_class()

PEP8 E501 - line too long (79 chars max)

::: pylib/pygments_override/pygments_override/extension.py:35
(Diff revision 1)
> -        lexers.guess_lexer_for_filename = get_lexer
> -        chunk_generator.guess_lexer_for_filename = get_lexer
> -        info('guess_lexer_for_filename() decoration done!')
>  
>      def shutdown(self):
> -        lexers.guess_lexer_for_filename = guess_lexer_for_filename
> +        set_diff_chunk_generator_class(self._original_diff_chunk_generator_class)

PEP8 E501 - line too long (79 chars max)

::: pylib/pygments_override/pygments_override/extension.py:40
(Diff revision 1)
> -        cache.clear()
> -        info('Django cache cleared!')
> +        overrides = self.settings['overrides'].splitlines()
> +        return dict([override.split('=') for override in overrides])

this code assumes the configuration string is well-formed.

if it isn't (eg, it doesn't contain an equal symbols), the extension will fail to load and you cannot get to the configure page to fix.

::: pylib/pygments_override/pygments_override/overridable_chunk_generator.py:50
(Diff revision 1)
> +            lexer = None
> +        return lexer
> +
> +_overrides = {}
> +
> +def set_overrides(overrides):

PEP8 E302 - need 2 blank lines before defs
Attachment #8744895 - Flags: review?(glob)
> I'm curious to know what makes pushing a complete new commit your preference?

I'm working this way with my history in hg:

  1. I checkout the commit.
  2. I patch it.
  3. I rebase further changes to stay over these new changes.
  4. I edit the history and fuse the patch with the original revision.

So if I operate the same way with the refactor, I'm loosing the original approach and I was not sure about which solution was the best option as the latest is overriding a private method so I wanted to keep that commit untouched in case we need to recover it.
Attachment #8744195 - Flags: review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

clearing this review - later revisions of this file exist in the patchset.
Attachment #8744196 - Flags: review?(glob)
https://reviewboard.mozilla.org/r/48399/#review45747

> this indentation triggers PEP8 E128: continuation line under-indented for visual indent

Well, these are the alternatives:

```python
url(r'^$', 'reviewboard.extensions.views.configure_extension', {
                                                                'ext_class': PygmentsOverride,
                                                                'form_class': PygmentsOverrideSettingsForm
                                                               }
),
```

Or:

```python
url(
    r'^$',
    'reviewboard.extensions.views.configure_extension',
    {
        'ext_class': PygmentsOverride,
        'form_class': PygmentsOverrideSettingsForm
    }
),
```

I don't really like any of them but I prefer the latter one. What I'm inclined to do is to suppress the error honoring "_practicality beats purity_" and for the sake of readability as the original form is a well known Django idiom.

What do you prefer, @glob?
https://reviewboard.mozilla.org/r/48399/#review45747

> Well, these are the alternatives:
> 
> ```python
> url(r'^$', 'reviewboard.extensions.views.configure_extension', {
>                                                                 'ext_class': PygmentsOverride,
>                                                                 'form_class': PygmentsOverrideSettingsForm
>                                                                }
> ),
> ```
> 
> Or:
> 
> ```python
> url(
>     r'^$',
>     'reviewboard.extensions.views.configure_extension',
>     {
>         'ext_class': PygmentsOverride,
>         'form_class': PygmentsOverrideSettingsForm
>     }
> ),
> ```
> 
> I don't really like any of them but I prefer the latter one. What I'm inclined to do is to suppress the error honoring "_practicality beats purity_" and for the sake of readability as the original form is a well known Django idiom.
> 
> What do you prefer, @glob?

the second one.  the first exceeds the 79 char limit.

> What I'm inclined to do is to suppress the error honoring [..]

i hear you and don't totally disagree, but the current plan is to be pep8 compliant.
https://reviewboard.mozilla.org/r/48717/#review45547

> Rather than fetching and storing these values, we should access them directly from settings every time we need them. When you update the settings through Review Board's admin panel it has code which will automatically synchronize the new settings to every web head, and future access will return the new values. This would allow us to update the settings at runtime without re-enabling the extension.
> 
> You can even have the settings access take place directly in the `OverrideableDiffChunkGenerator`. In order to get a reference to the instantiated `PygmentsOverride` `Extension` object you first need to fetch the extension manager, and it will return you the extension. e.g.:
> ```Python
> ext = get_extension_manager().get_enabled_extension(
>     'pygments_override.extension.PygmentsOverride')
> ```
> We have examples where we do this currently[1] in an extension (which will show you the import paths).
> 
> After you have the extension object you can access `ext.settings` for the up to date values.
> 
> [1] https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0a2fb6bd2b358add301a76d47fe2dc22bcc426bf/pylib/mozreview/mozreview/autoland/resources.py#163

> Rather than fetching and storing these values, we should access them directly from settings every time we need them. When you update the settings through Review Board's admin panel it has code which will automatically synchronize the new settings to every web head, and future access will return the new values. This would allow us to update the settings at runtime without re-enabling the extension.

Actually, each time a setting change, the extension is re-initialized but I ignore why so I prefer this way.
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/3-4/
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/4-5/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/4-5/
Comment on attachment 8744895 [details]
MozReview Request: Bug 1255876 - Refactored to set a custom diff chunk generator instead of monkeypatching pygments; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48717/diff/1-2/
(In reply to Salvador de la Puente González [:salva] from comment #38)
> > I'm curious to know what makes pushing a complete new commit your preference?
> 
> I'm working this way with my history in hg:
> 
>   1. I checkout the commit.
>   2. I patch it.
>   3. I rebase further changes to stay over these new changes.
>   4. I edit the history and fuse the patch with the original revision.
> 
> So if I operate the same way with the refactor, I'm loosing the original
> approach and I was not sure about which solution was the best option as the
> latest is overriding a private method so I wanted to keep that commit
> untouched in case we need to recover it.

We highly prefer commits are amended to fix issues brought up in review
rather than creating new commits which fix the issues. Any of the commits
you push to the review repository are stored there permanently, so you
don't need to worry about losing the history if we want to roll back.

It would be a lot cleaner and easier to review if you could roll the latest
push with the extra commit and fixes into the related commits.
Flags: needinfo?(salva)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/5-6/
Attachment #8744196 - Attachment description: MozReview Request: Bug 1255876 - Decorating pygments guess_lexer_for_filename() to allow overrides per file extension; r=glob → MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/5-6/
Comment on attachment 8745368 [details]
MozReview Request: Bug 1255876 - Applied yapf (https://github.com/google/yapf) to be compliant with PEP8; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49013/diff/1-2/
Attachment #8744895 - Attachment is obsolete: true
Attachment #8744895 - Flags: review?(glob)
Attachment #8745368 - Flags: review?(glob)
Comment on attachment 8745368 [details]
MozReview Request: Bug 1255876 - Applied yapf (https://github.com/google/yapf) to be compliant with PEP8; r=glob

https://reviewboard.mozilla.org/r/49013/#review45953

unfortunately the code style used by yapf isn't consistent with the rest of mozreview's code (eg. the formatting of import lines).
please just fix up the issues that flake8 raises instead of applying yapf.

fwiw i would *love* to see yapf used everywhere to enforce style in a clearly documented way, but that's an a change that needs to be discussed with the team before we can do it.
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

this doesn't need a second review
Attachment #8744195 - Flags: review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

https://reviewboard.mozilla.org/r/48395/#review45959

::: pylib/pygments_override/pygments_override/extension.py:2
(Diff revision 6)
> +import logging
> +from os.path import splitext

nit: splittext is not used
Attachment #8744196 - Flags: review?(glob)
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

https://reviewboard.mozilla.org/r/48399/#review45963

::: pylib/pygments_override/pygments_override/forms.py:18
(Diff revision 6)
> +    )
> +
> +    def clean_overrides(self):
> +        overrides = self.normalize_overrides(self.cleaned_data['overrides'])
> +        # The regexp stands for a list of break-line ended .ext=type pairs.
> +        if not re.match('^(\.\w+=\w+\n)*$', overrides, flags=re.MULTILINE):

if an invalid lexer is provided (eg. .txt=josn) it will be silently ignored by the chunk generator.

instead we should validate the lexer provided is a valid name according to pygments, and throw an error if it isn't.
Attachment #8744198 - Flags: review?(glob)
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/4-5/
Attachment #8744195 - Flags: review?(glob)
Attachment #8744196 - Flags: review?(glob)
Attachment #8744198 - Flags: review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/6-7/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/6-7/
Attachment #8745368 - Attachment is obsolete: true
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

clearing review request - no changes
Attachment #8744195 - Flags: review?(glob)
https://reviewboard.mozilla.org/r/48399/#review45963

> if an invalid lexer is provided (eg. .txt=josn) it will be silently ignored by the chunk generator.
> 
> instead we should validate the lexer provided is a valid name according to pygments, and throw an error if it isn't.

I hope you like it. I think it could be better but it is good enough now (I hope...)
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/5-6/
Attachment #8744195 - Flags: review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/7-8/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/7-8/
I did roll the latest changes dropping the PEP8 suggestions related to import to preserve our way. Let me know how does it look now.
Flags: needinfo?(salva)
:glob, do you think it is ready to land?
Flags: needinfo?(glob)
(In reply to Salvador de la Puente González [:salva] from comment #65)
> :glob, do you think it is ready to land?

sorry, i didn't have time to review it yesterday.
Flags: needinfo?(glob)
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

again, this doesn't need a second review.  please update your commit message to direct the review to smacleod not me.
Attachment #8744195 - Flags: review?(glob)
have you made any progress on the upstream version?
Flags: needinfo?(salva)
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

clearing smac's review; it's being automatically carried forward but a lot has changed since his r+
Attachment #8744198 - Flags: review+
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

https://reviewboard.mozilla.org/r/48399/#review47151

::: pylib/pygments_override/pygments_override/forms.py:61
(Diff revision 8)
> +            if len(sug) == 0:
> +                return u''
> +            elif len(sug) == 1:
> +                return u'(perhaps you meant %s)' % sug[0]
> +            else:
> +                return u'(perhaps you meant one of %s)' % ', '.join(sug)

i like that you return suggestions :)
Attachment #8744198 - Flags: review?(glob)
Attachment #8744196 - Flags: review?(glob)
Attachment #8744198 - Flags: review+
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48393/diff/6-7/
Attachment #8744195 - Flags: review?(glob)
Attachment #8744196 - Flags: review?(glob)
Attachment #8744198 - Flags: review+ → review?(glob)
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48395/diff/8-9/
Comment on attachment 8744198 [details]
MozReview Request: Bug 1255876 - Adding override setting to the extension to provide the list of overrides; r=glob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48399/diff/8-9/
Comment on attachment 8744195 [details]
MozReview Request: Bug 1255876 - Adding the new pygments plugin boilerplate; r=glob

looks like it's simplest if i just r+ this myself
Attachment #8744195 - Flags: review?(glob)
Attachment #8744195 - Flags: review+
Comment on attachment 8744196 [details]
MozReview Request: Bug 1255876 - Overriding the diff chunk generator to take into account explicit per-extension overrides; r=glob

https://reviewboard.mozilla.org/r/48395/#review47981

r=glob

this needs to be squashed before it can be landed.  i'll do both for you.
Attachment #8744196 - Flags: review?(glob) → review+
Attachment #8744198 - Flags: review?(glob) → review+
land the extension in https://hg.mozilla.org/hgcustom/version-control-tools/rev/e391b4b78fa2

as per comment 2, comment 4 and comment 13 this needs to happen upstream; leaving open.
I'm wondering if enabling automatic configuration (like mozreview plugin) or if you prefer to leave it manual.
Flags: needinfo?(salva) → needinfo?(glob)
(In reply to Salvador de la Puente González [:salva] from comment #77)
> I'm wondering if enabling automatic configuration (like mozreview plugin) or
> if you prefer to leave it manual.

manual is fine
Flags: needinfo?(glob)
Depends on: 1273177
I don't think leaving this open is right, since for all intents and purposes this is fixed in MozReview.  I've filed bug 1273690 for this, in the Review Board: Upstream component.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: