Fix order issues #208

Closed
HugoGiraudel opened this Issue Sep 19, 2014 · 29 comments

Comments

Projects
None yet
3 participants
@HugoGiraudel
Member

HugoGiraudel commented Sep 19, 2014

Because we are dealing with file parsing in a totally asynchronous way, we can't know which file will be parsed first, which has a direct repercussion on the order used to display items on screen.

We should probably do:

  • Alphabetically for groups (sorted by alias if exist, else name);
  • Alphabetically for types (function, mixin, placeholder, variable);
  • Alphabetically for file names;
  • Source order for items.

What do you say?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

We already discussed this at many places if I'm right…

#71
#160 (comment)

Member

valeriangalliat commented Sep 19, 2014

We already discussed this at many places if I'm right…

#71
#160 (comment)

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 19, 2014

Member

Yet we still haven't fixed it. Let's do it for real.

Member

HugoGiraudel commented Sep 19, 2014

Yet we still haven't fixed it. Let's do it for real.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

So to sum up #71, the discussed order would be:

  1. Group
  2. Type
  3. Filename
  4. Position in file
Member

valeriangalliat commented Sep 19, 2014

So to sum up #71, the discussed order would be:

  1. Group
  2. Type
  3. Filename
  4. Position in file
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 19, 2014

Member

Updated my initial comment.

Member

HugoGiraudel commented Sep 19, 2014

Updated my initial comment.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

👍

For this we need the filename and item source order information in the data interface (the sorting would be some kind of post processing).

What about adding filename and position keys to each data items? filename is obvious (though we have to decide if it's relative to the source directory or absolute, but that won't affect the sorting), and position would be the item number in the file.

Edit: we already have item.file (and it's relative).

Member

valeriangalliat commented Sep 19, 2014

👍

For this we need the filename and item source order information in the data interface (the sorting would be some kind of post processing).

What about adding filename and position keys to each data items? filename is obvious (though we have to decide if it's relative to the source directory or absolute, but that won't affect the sorting), and position would be the item number in the file.

Edit: we already have item.file (and it's relative).

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 19, 2014

Member

We need and item.position or item.index then.

Member

HugoGiraudel commented Sep 19, 2014

We need and item.position or item.index then.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member
diff --git a/src/file.js b/src/file.js
index 171e61d..3a3af97 100644
--- a/src/file.js
+++ b/src/file.js
@@ -122,6 +122,8 @@ exports = module.exports = {
       return exports.file.read(file, 'utf-8').then(function (code) {
         var data = parser.parse(code);

+        var i = 0;
+
         // Merge in from which file the comments where loaded.
         Object.keys(data).forEach(function (key) {
           data[key].forEach(function (item) {
@@ -129,6 +131,8 @@ exports = module.exports = {
               path: path.relative(exports.folder.base, file),
               name: path.basename(file, '.scss')
             };
+
+            item.index = i++;
           });
         });

What do you think?

Edit: this isn't actually giving the item index in the file, because it's already indexed by type (I guess) - and it's assuming the parser returns the items in the file order for each type. Though I think it would do the job for the sorting.

Member

valeriangalliat commented Sep 19, 2014

diff --git a/src/file.js b/src/file.js
index 171e61d..3a3af97 100644
--- a/src/file.js
+++ b/src/file.js
@@ -122,6 +122,8 @@ exports = module.exports = {
       return exports.file.read(file, 'utf-8').then(function (code) {
         var data = parser.parse(code);

+        var i = 0;
+
         // Merge in from which file the comments where loaded.
         Object.keys(data).forEach(function (key) {
           data[key].forEach(function (item) {
@@ -129,6 +131,8 @@ exports = module.exports = {
               path: path.relative(exports.folder.base, file),
               name: path.basename(file, '.scss')
             };
+
+            item.index = i++;
           });
         });

What do you think?

Edit: this isn't actually giving the item index in the file, because it's already indexed by type (I guess) - and it's assuming the parser returns the items in the file order for each type. Though I think it would do the job for the sorting.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 19, 2014

Member

That should do the trick. We need to add a little task to sort the tree. Maybe in sassdoc-indexer? Or should it be in the core?

Member

HugoGiraudel commented Sep 19, 2014

That should do the trick. We need to add a little task to sort the tree. Maybe in sassdoc-indexer? Or should it be in the core?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

You wan't this to be the theme's responsibility, not in the core?

Member

valeriangalliat commented Sep 19, 2014

You wan't this to be the theme's responsibility, not in the core?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 19, 2014

Member

Fair point, core.

Member

HugoGiraudel commented Sep 19, 2014

Fair point, core.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

Okay, so this can be plugged just after parser.postProcess.

Member

valeriangalliat commented Sep 19, 2014

Okay, so this can be plugged just after parser.postProcess.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

How do you want to sort by group before the type since the data is already indexed by type? Maybe provide a flat data for 2.0?

Member

valeriangalliat commented Sep 19, 2014

How do you want to sort by group before the type since the data is already indexed by type? Maybe provide a flat data for 2.0?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 19, 2014

Member

BTW we can't sort by group alias at SassDoc's core level since the aliases are handled by the theme.

Member

valeriangalliat commented Sep 19, 2014

BTW we can't sort by group alias at SassDoc's core level since the aliases are handled by the theme.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment

@valeriangalliat valeriangalliat self-assigned this Sep 19, 2014

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 20, 2014

Member

So, I am a little confused about this one. What is your suggestion at this point @valeriangalliat?

Member

HugoGiraudel commented Sep 20, 2014

So, I am a little confused about this one. What is your suggestion at this point @valeriangalliat?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 20, 2014

Member

To include this in 1.8 without breaking the API, we can't sort by group before type (since the data is indexed by type). So right now the data will be sorted by type (in the index), then same order as said above.

For 2.0, we can allow ourselves to break the API, thus making the data interface a flat array or anything, and be free to sort the way we want.

Member

valeriangalliat commented Sep 20, 2014

To include this in 1.8 without breaking the API, we can't sort by group before type (since the data is indexed by type). So right now the data will be sorted by type (in the index), then same order as said above.

For 2.0, we can allow ourselves to break the API, thus making the data interface a flat array or anything, and be free to sort the way we want.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 20, 2014

Member

Alright.

Member

HugoGiraudel commented Sep 20, 2014

Alright.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
Member

valeriangalliat commented Sep 20, 2014

Updated sorter.

develop...sorter

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 20, 2014

Member

Build passing. If anybody wants to review before merging…

Member

valeriangalliat commented Sep 20, 2014

Build passing. If anybody wants to review before merging…

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
Member

HugoGiraudel commented Sep 20, 2014

@FWeinb, @pascalduez, please.

@HugoGiraudel HugoGiraudel modified the milestone: 1.8 Sep 20, 2014

valeriangalliat added a commit that referenced this issue Sep 21, 2014

Merge pull request #211 from SassDoc/sorter
Sort data according to #208 discussion
@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 21, 2014

Member

See comments on the PR page. Merged!

Member

valeriangalliat commented Sep 21, 2014

See comments on the PR page. Merged!

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Sep 24, 2014

Member

I added the line number of a found comment to the context so maybe we could use that to do the sorting?

Member

FWeinb commented Sep 24, 2014

I added the line number of a found comment to the context so maybe we could use that to do the sorting?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 24, 2014

Member

I think @valeriangalliat already took care of file order, but it might not hurt to double check.

Member

HugoGiraudel commented Sep 24, 2014

I think @valeriangalliat already took care of file order, but it might not hurt to double check.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 24, 2014

Member

I'm comparing on item index (and the index property was added only for sorting purposes). Now we have the line, we can just sort on line and remove the index. What du you think?

Member

valeriangalliat commented Sep 24, 2014

I'm comparing on item index (and the index property was added only for sorting purposes). Now we have the line, we can just sort on line and remove the index. What du you think?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 24, 2014

Member

I'm all in favor.

Member

HugoGiraudel commented Sep 24, 2014

I'm all in favor.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 24, 2014

Member

We didn't publish the version with index in the API, right?

Member

valeriangalliat commented Sep 24, 2014

We didn't publish the version with index in the API, right?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 24, 2014

Member

Not yet. Tomorrow.

Member

HugoGiraudel commented Sep 24, 2014

Not yet. Tomorrow.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Sep 24, 2014

Member

If you want to update it, it's now or never! :P

Member

HugoGiraudel commented Sep 24, 2014

If you want to update it, it's now or never! :P

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Sep 24, 2014

Member

Summary of the IRC discussion: we release with index for 1.8 (but index is only internal use, not public API), then item.context.line comes with 1.9 and we use it for the sorting, and remove index.

I'm creating an issue for this 1.9 patch.

Member

valeriangalliat commented Sep 24, 2014

Summary of the IRC discussion: we release with index for 1.8 (but index is only internal use, not public API), then item.context.line comes with 1.9 and we use it for the sorting, and remove index.

I'm creating an issue for this 1.9 patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment