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

DocumentationComment: Only accept `position` instead of `range` in `__init__` #2646

Closed
Makman2 opened this Issue Aug 20, 2016 · 7 comments

Comments

4 participants
@Makman2
Member

Makman2 commented Aug 20, 2016

As indent is already implemented as a field, the complete range is fully constructable now when giving a start position.

So this in __init__ should do the trick:

        self._assembled = self.assemble()
        self.range = TextRange.from_values(
            position.line,
            position.column,
            position.line + self._assembled.count('\n'),
            len(self._assembled) + self._assembled.rfind('\n'))

As assemble() needs to be executed for that, this value could be cached and used on call:

def __init__(...):
    self._assembled = self._assemble()

def assemble(self):
    return self._assembled

def _assemble(self):
    # here lies now the code that does the assembling from the original `assemble()`
@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Aug 20, 2016

Collaborator

Thanks for reporting this issue! A coalaian will look at it soon.

Collaborator

gitmate-bot commented Aug 20, 2016

Thanks for reporting this issue! A coalaian will look at it soon.

@SanketDG SanketDG self-assigned this Aug 20, 2016

@gitmate-bot gitmate-bot referenced this issue May 24, 2017

Merged

projects: Add cobot enhancement project #289

0 of 2 tasks complete

@SanketDG SanketDG assigned damngamerz and unassigned SanketDG Jun 2, 2017

@damngamerz

This comment has been minimized.

Show comment
Hide comment
@damngamerz

damngamerz Jun 13, 2017

Member

@Makman2 I still couldn't understand about the complete constructible full range, can you like explain me with an example. Right now i understand how TextRange works.

Member

damngamerz commented Jun 13, 2017

@Makman2 I still couldn't understand about the complete constructible full range, can you like explain me with an example. Right now i understand how TextRange works.

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 13, 2017

Member

I think I meant instead of taking a range in DocumentationComment.__init__, just take in a position instead. It's enough to describe what TextRange the comment will occupy.

self.range = TextRange.from_values(
            position.line,
            position.column,
            position.line + self._assembled.count('\n'),
            len(self._assembled) + self._assembled.rfind('\n'))
Member

Makman2 commented Jun 13, 2017

I think I meant instead of taking a range in DocumentationComment.__init__, just take in a position instead. It's enough to describe what TextRange the comment will occupy.

self.range = TextRange.from_values(
            position.line,
            position.column,
            position.line + self._assembled.count('\n'),
            len(self._assembled) + self._assembled.rfind('\n'))
@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 13, 2017

Member

So it's just a simplification for the constructor.

Member

Makman2 commented Jun 13, 2017

So it's just a simplification for the constructor.

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 13, 2017

Member

I hope this issue is still valid^^

Member

Makman2 commented Jun 13, 2017

I hope this issue is still valid^^

@damngamerz

This comment has been minimized.

Show comment
Hide comment
@damngamerz

damngamerz Jun 14, 2017

Member

@Makman2 I doubt the validity of this issue.Anyway I tried amending it, Works well check this damngamerz@ccb7b65
This majorily will break all the tests of DocumentationComment & DocumentationExtraction I have to re-write them. That's not the issue, issue is do we really need this ??

Member

damngamerz commented Jun 14, 2017

@Makman2 I doubt the validity of this issue.Anyway I tried amending it, Works well check this damngamerz@ccb7b65
This majorily will break all the tests of DocumentationComment & DocumentationExtraction I have to re-write them. That's not the issue, issue is do we really need this ??

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 14, 2017

Member

I think providing a full range means we throw away information from it or stuff does break inside DocumentationComment if something is wrong. Ideally this would be good to have.

I'm usually the type of guy that gets angry when he needs to pass obsolete information, as often it's not directly available ;)

Member

Makman2 commented Jun 14, 2017

I think providing a full range means we throw away information from it or stuff does break inside DocumentationComment if something is wrong. Ideally this would be good to have.

I'm usually the type of guy that gets angry when he needs to pass obsolete information, as often it's not directly available ;)

@damngamerz damngamerz referenced this issue Jun 16, 2017

Merged

DocumentationComment.py: Accept `position` #4368

2 of 2 tasks complete

damngamerz added a commit to damngamerz/coala that referenced this issue Jun 25, 2017

DocumentationComment.py: Accept `position`
DocumentationComment accept `position` instead
of `range` in `__init__`.
Cache `assemble()` to construct full `range`.
Amend tests to comply with changes.
Closes coala#2646

@rultor rultor closed this in #4368 Jun 25, 2017

S2606 added a commit to S2606/coala that referenced this issue Jun 27, 2017

DocumentationComment.py: Accept `position`
DocumentationComment accept `position` instead
of `range` in `__init__`.
Cache `assemble()` to construct full `range`.
Amend tests to comply with changes.
Closes coala#2646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment