Klokane/annotation with column line info #653

klokane · 2018-10-02T14:54:08Z

Source code ready for review

klokane · 2018-10-02T14:55:01Z

src/RefractAPI.cc

@@ -26,6 +26,8 @@
 #include "NamedTypesRegistry.h"
 #include "ConversionContext.h"

+#include "reporting.h"


remove - restful

klokane · 2018-10-02T14:55:42Z

src/RefractSourceMap.cc

@@ -1,4 +1,6 @@
 #include "RefractSourceMap.h"
+#include "reporting.h"


remove - restful

src/SourceMapUtils.h

klokane · 2018-10-02T14:57:10Z

src/reporting.h

@@ -11,6 +11,7 @@

 #include "drafter.h"
 #include "SourceAnnotation.h"
+#include "reporting.h"


remove - mistake

klokane · 2018-10-02T14:58:17Z

test/draftertest.h

@@ -103,10 +103,11 @@ namespace draftertest

    struct FixtureHelper {

-        static std::unique_ptr<refract::IElement> parseAndSerialize(
-            snowcrash::ParseResult<snowcrash::Blueprint>& blueprint, const drafter::WrapperOptions& options)
+        static std::unique_ptr<refract::IElement> parseAndSerialize(const std::string& source,


remove - place inside handleResultJSON()

kylef · 2018-10-02T20:32:30Z

tools/update-fixtures.sh

@@ -0,0 +1,19 @@
+#!/bin/sh
+
+for i in $(./bin/test-libdrafter | grep Filename | sed -e 's/^  Filename: //' | sed -e 's/\x1B\[[0-9;]*[JKmsu]//g')


You can already do env GENERATE=1 make test to update fixtures, was you unaware of that?

Oh, I remember there were some discussion related it is not working.
So I probably miss info it is working again.

Ok, so i will remove this script.

klokane · 2018-10-02T21:06:06Z

@apiaryio/adt ready for review

pksunkara

The line and column info should be optional (a new option to drafter_parse) and it should be defaulted to false. @kylef Can you confirm?

We will have no need to change all the existing fixtures.

pksunkara · 2018-10-04T11:56:50Z

test/draftertest.h

    {                                                                                                                  \
-        REQUIRE(FixtureHelper::handleResultJSON(wrapper, "test/fixtures/" category "/" name, options, mustBeOk));      \
+        REQUIRE(FixtureHelper::handleResultJSON("test/fixtures/mson/" name, MSONTestOptions, mustBeOk));               \


Like below,

- MSONTestOptions + drafter::WrapperOptions(false, true)

I have not seen in assignment it should be optional. Assignment reference fury-adapter-swagger apiaryio/fury-adapter-swagger#211

It is reason why I mean it should not be optional, but if it should be it is not problem to adapt it.

kylef · 2018-10-05T15:53:02Z

The line and column info should be optional (a new option to drafter_parse) and it should be defaulted to false. @kylef Can you confirm?

In the Swagger 2 parser line and number is always there. I don't see any benefit of it being optional because this is something that most consumers want. Do you have some use-case in mind where you would want to disable line and column numbers for annotation source maps?

kylef · 2018-10-05T16:02:24Z

test/fixtures/syntax/issue-350.json

+                        },
+                        "column": {
+                          "element": "number",
+                          "content": 1


As per the spec, the column should be zero-indexed. Since this is empty line this should be 0?

https://api-elements.readthedocs.io/en/latest/element-definitions.html#source-map

Also, given the ADD I think the source map should not end on the start of line 9, but end of line 8? Maybe larger issue to deal with than in the scope of this PR though.

I use algorithm which was already there for -l switch. So it is wrong in original code.
I will look at it

Actually, I don't see where in the spec the line or column is said to be zero-indexed. Only the byte offset of the source map is said to be zero-indexed. The column and line attributes are actually very undefined per spec.

Hmm, you are right. We should clarify in the spec. Should we go for 1-indexed or 0-indexed?

#653 (comment)

column and line numbers are good old natural numbers, and I would keep that. Natural numbers, in my world, start from 1.

test/test-RefractDataStructureTest.cc

tjanc · 2018-10-08T07:23:12Z

src/reporting.cc

@@ -134,7 +70,7 @@ namespace
                if (useLineNumbers) {

                    AnnotationPosition annotationPosition;
-                    GetLineFromMap(linesEndIndex, *it, annotationPosition);
+                    ::GetLineFromMap(linesEndIndex, *it, annotationPosition);


Wouldn't it be better to use a using declaration or qualify by namespace instead of referencing a global symbol via :: prefix?

Can be removed, that is rest after some "experiments" :)

tjanc

Apart from pointed out improvements, please introduce unit tests.

tjanc · 2018-10-08T07:24:47Z

src/SourceMapUtils.h

+     *  \param range Character index mapping as input
+     *  \param out Position of the given range as output
+     */
+    void GetLineFromMap(const std::vector<size_t>& linesEndIndex, const mdp::Range& range, AnnotationPosition& out);


Why is out not returned by value here? It's just four integers.

tjanc · 2018-10-08T07:28:21Z

src/SourceMapUtils.h

+     *  \param source Source data
+     *  \param out Vector containing indexes of all end line character in source
+     */
+    void GetLinesEndIndex(const std::string& source, NewLinesIndex& out);


Why do we need to fill a vector? Would it be possible/desirable to provide a callback called instead of push_backing?

tjanc · 2018-10-08T07:29:04Z

src/SourceMapUtils.cc

+        out.toColumn = 0;
+
+        // Finds starting line and column position
+        annotationPositionIt = std::upper_bound(linesEndIndex.begin(), linesEndIndex.end(), range.location) - 1;


A few suggestions:

auto annotationPosition = std::upper_bound(...) ...; instead of declaration on line 10.

out is completely reset on lines 12-15; consider returning AnnotationPosition instead.

linesEndIndex is a vector, but this is not vector-specific code; maybe use iterator templates instead?

the - 1 at the end of line 18 is something I have noticed after third reading. Please use std::prev or std::advance to both make the operation more visible and generalize to non-random_access_iterator.

tjanc · 2018-10-08T07:39:55Z

src/SourceMapUtils.cc

+            out.toLine = std::distance(linesEndIndex.begin(), annotationPositionIt) + 1;
+            out.toColumn = (range.location + range.length) - *annotationPositionIt + 1;
+
+            if (*(annotationPositionIt + 1) == (range.location + range.length)) {


suggesting std::next instead of + 1 on iterator

tjanc · 2018-10-08T07:44:20Z

src/SourceMapUtils.h

+
+    /** structure contains starting and ending position of a error/warning. */
+    struct AnnotationPosition {
+        size_t fromLine;


Please add a default initializer for each member (size_t fromLine = 0).

src/SourceMapUtils.h

tjanc · 2018-10-08T07:45:53Z

src/SourceMapUtils.h

+namespace drafter
+{
+
+    typedef std::vector<size_t> NewLinesIndex;


Prefer using declaration instead of typedefs.

src/SourceMapUtils.h

pksunkara · 2018-10-10T10:00:10Z

In the Swagger 2 parser line and number is always there. I don't see any benefit of it being optional because this is something that most consumers want. Do you have some use-case in mind where you would want to disable line and column numbers for annotation source maps?

When people need sourcemaps to manipulate stuff but it's not a user interface so they don't need line and column info.

klokane · 2018-10-22T13:47:21Z

Oh, I realize code will not working correctly with utf-8 because it count byte positions instead of char positions. Will going to fix it.

tjanc

Well addressed, just two more little-things.

tjanc · 2018-11-08T15:37:08Z

src/ConversionContext.h

@@ -34,7 +38,10 @@ namespace drafter
            return registry;
        }

-        ConversionContext(const WrapperOptions& options) : options(options) {}
+        inline const NewLinesIndex& GetNewLinesIndex() const


Why inline? Fun fact: inline causes GCC to produce a weak symbol, that's all.

CHANGELOG.md

pksunkara · 2018-11-09T16:06:21Z

We still haven't made the column/line info optional

pksunkara · 2018-11-09T16:05:25Z

CHANGELOG.md

+## master
+
+* Add column/line info to anotations source maps 
+* change start column index from 1 to 0 in reporting. (while `-l` switch is used)


Why is this done? This is UI. We need to use index starting from 1. @kylef What do you think?

https://github.com/apiaryio/drafter/pull/653/files/3217ac2c099085b7bfd2640e1ad91751287697e2#r223059516

I think you mean the -u flag?

-u, --use-line-num use line and row number instead of character index when printing annotation

I don't think this is nessecerily done in the UI, it is an explanation that -u flag will now behave differently than before because the same logic is used for column/line numbers.

Should -u use 1-indexed or 0-indexed? Looking at other CLI tools it seems 1-indexed is the standard but this is up for discussion.

https://github.com/apiaryio/drafter/pull/653/files/3217ac2c099085b7bfd2640e1ad91751287697e2#r223059516

Based on my reading of the spec Pavan raised a valid point. There is actually no need to change to zero-based indexing for column and line attributes of source maps; I initially also misread the spec, because it talks about zero-based indexing - of the byte offset however, not column and line numbers. All in all, column and line numbers are good old natural numbers, and I would keep that. Natural numbers, in my world, start from 1.

Ok, so do you want to return back implementation to one-indexed like in original code?

I made unit tests related to indexing 25 days ago. It was clear from this code how indexing will be implemented. I asked for review, nobody commented it...
f255865

klokane · 2018-11-09T16:12:14Z

Yes, we did not it optional.
There were discussion related to this and there was no decision make it optional

tjanc · 2018-11-09T16:24:09Z

We still haven't made the column/line info optional

I was convinced we agreed not to make it optional. If output size is a worry, we should tackle the problem more head on, providing a flag for output size optimization.

Maybe another PR "Add flag to disable column/line emission in source maps"?

…lumn/line

klokane · 2018-11-13T15:50:54Z

@apiaryio/adt so it is updated against master (include cmake build) and colums are indexed from 1.
So please for review

resolved

klokane added the DO NOT MERGE label Oct 2, 2018

klokane commented Oct 2, 2018

View reviewed changes

src/RefractSourceMap.cc Outdated

@@ -1,4 +1,6 @@

#include "RefractSourceMap.h"

#include "reporting.h"

Copy link

Contributor Author

klokane Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove - restful

klokane commented Oct 2, 2018

View reviewed changes

src/SourceMapUtils.h Show resolved Hide resolved

klokane commented Oct 2, 2018

View reviewed changes

kylef reviewed Oct 2, 2018

View reviewed changes

klokane force-pushed the klokane/annotation-with-column-line-info branch from 59b01cb to 5273e8d Compare October 2, 2018 20:51

klokane removed the DO NOT MERGE label Oct 2, 2018

klokane force-pushed the klokane/annotation-with-column-line-info branch from 5273e8d to 3217ac2 Compare October 2, 2018 20:56

pksunkara suggested changes Oct 4, 2018

View reviewed changes

kylef reviewed Oct 5, 2018

View reviewed changes

tjanc reviewed Oct 8, 2018

View reviewed changes

test/test-RefractDataStructureTest.cc Show resolved Hide resolved

tjanc reviewed Oct 8, 2018

View reviewed changes

tjanc suggested changes Oct 8, 2018

View reviewed changes

klokane force-pushed the klokane/annotation-with-column-line-info branch 4 times, most recently from fe87c93 to 4e990f8 Compare November 6, 2018 12:58

tjanc previously requested changes Nov 8, 2018

View reviewed changes

klokane force-pushed the klokane/annotation-with-column-line-info branch from 4e990f8 to 3c9acd8 Compare November 8, 2018 16:06

pksunkara previously requested changes Nov 9, 2018

View reviewed changes

kylef mentioned this pull request Nov 9, 2018

Clarify source map line/column indexing apiaryio/api-elements#52

Open

klokane added 12 commits November 13, 2018 13:38

update: add Column/Line information for annotations

6a315e9

update: update tests for anotation extended info

ccb859e

chore: update fixtures according to changes

70420da

chore: remove restful header file

7a2146d

chore: update integration testr fixtures

d514785

chore: remove unused function

7626424

update: add unit test to cover map sourcemap ranges translation to co…

fa195df

…lumn/line

Update implementation

fa7a80a

Update fixtures

be04cd7

chore: update changelog

8998ab8

chore: update integration tests according to impl

8aaa8c4

chore: update cmake according to master

42e55bd

klokane force-pushed the klokane/annotation-with-column-line-info branch from 3c9acd8 to 42e55bd Compare November 13, 2018 12:54

klokane added 4 commits November 13, 2018 16:15

update: rewrite indexing of column from 1

cf98113

chore: update fixtures

8edc53f

chore: update integrations tests

a646130

chore: update changelog

3e078fa

kylef mentioned this pull request Nov 14, 2018

fix: Default in enum parameters should be enum element #658

Merged

tjanc approved these changes Nov 15, 2018

View reviewed changes

klokane merged commit 1facd70 into master Nov 15, 2018

klokane deleted the klokane/annotation-with-column-line-info branch November 15, 2018 15:07

		@@ -1,4 +1,6 @@
		#include "RefractSourceMap.h"
		#include "reporting.h"

		@@ -0,0 +1,19 @@
		#!/bin/sh

		for i in $(./bin/test-libdrafter \| grep Filename \| sed -e 's/^ Filename: //' \| sed -e 's/\x1B\[[0-9;]*[JKmsu]//g')

Klokane/annotation with column line info #653

Klokane/annotation with column line info #653

Conversation

klokane commented Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klokane commented Oct 2, 2018

pksunkara left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylef commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara commented Oct 10, 2018

klokane commented Oct 22, 2018

tjanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klokane commented Nov 9, 2018

tjanc commented Nov 9, 2018

klokane commented Nov 13, 2018

klokane commented Oct 2, 2018 •

edited

Loading

pksunkara left a comment •

edited

Loading