Skip to content
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

add optionnable sorting capability #2

Closed
wants to merge 16 commits into from

Conversation

quazardous
Copy link
Contributor

@quazardous quazardous commented Jan 6, 2017

hey I gave it a try ;p
Ok it took me 3 hours to write 30 lines ;p It's my first Node JS stuff ever (like a virgin)...

I had to introduce a technical _raw raw attribute to the file structure to sort formatted data...

I not very pleased with this double data structure but it works...

I've added some basic test and fix errors induced by the new attribute...

give me some feedback !!

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage decreased (-4.5%) to 95.536% when pulling 8e99e96 on quazardous:master into 903dcc2 on coderaiser:master.

@quazardous
Copy link
Contributor Author

see coderaiser/cloudcmd#101

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 99.107% when pulling a6cd387 on quazardous:master into 903dcc2 on coderaiser:master.

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d4b676b on quazardous:master into 903dcc2 on coderaiser:master.

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage decreased (-0.9%) to 99.145% when pulling caaaeb5 on quazardous:master into 903dcc2 on coderaiser:master.

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage decreased (-0.9%) to 99.145% when pulling bffb42b on quazardous:master into 4d97dd0 on coderaiser:master.

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d404ad3 on quazardous:master into 4d97dd0 on coderaiser:master.

@quazardous
Copy link
Contributor Author

done !

@coderaiser
Copy link
Owner

coderaiser commented Jan 8, 2017

Thank you, code a little bit messy but idea understandable :). I need to think about it a little bit.

res = -res;
}
return res;
}, array);
});
Copy link
Owner

Choose a reason for hiding this comment

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

I think sort-by can be used for sorting. It can simplify code a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not but no commit since 1 year ?

Copy link
Owner

Choose a reason for hiding this comment

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

sort-on uses dot-prop which is es2015 (and it requires to up major version of all modules which uses readify and drop support of old node.js versions). Maybe you know other modules which does the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort by seams very straightforward. We could fork it and write a backport with 'classic' attribute access.

@quazardous
Copy link
Contributor Author

Yes it's messy ...
But I lack background with node js so I ve mainly copycat your code trying to avoid using more 'agile' prog pattern :o

@quazardous
Copy link
Contributor Author

quazardous commented Jan 8, 2017

We could add a formatting dedicated function ( in the squad stack line 202 ) to avoid having the raw attribute condition and manipulate only raw items internally.
The last function would be a formatOutput ish function.

@quazardous quazardous mentioned this pull request Jan 8, 2017
@quazardous quazardous closed this Jan 8, 2017
@quazardous quazardous reopened this Jan 8, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1d6606a on quazardous:master into 4d97dd0 on coderaiser:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1d6606a on quazardous:master into 4d97dd0 on coderaiser:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 97a8903 on quazardous:master into 4d97dd0 on coderaiser:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 97a8903 on quazardous:master into 4d97dd0 on coderaiser:master.

@coveralls
Copy link

coveralls commented Jan 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d38a7ea on quazardous:master into 4d97dd0 on coderaiser:master.

const cmpCallbacks = {
'numeric': (a, b) => (+a - +b),
'local_string': (a, b) => a.localeCompare(b.attr),
'default': (a, b) => (a > b ? 1 : -1),
Copy link
Owner

Choose a reason for hiding this comment

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

What difference between default and numeric sorting?
What purpose of localeCompare? As I understand from mdn without locales argument it will work in the same way as default sorting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not much, it enforces the numeric cast with the dir value. But since I've switched formatting it maybe useless now.

localCompare is for accents. I'm french and I deal with accentuated letters like é è ë ê. Those letters have the same rank than e. Standard comparison put them after z.

http://www.jstips.co/en/sorting-strings-with-accented-characters/

Copy link
Owner

Choose a reason for hiding this comment

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

localCompare is for accents. I'm french and I deal with accentuated letters like é è ë ê.

That is interesting. Did not know about it.
Can you show me please how ls -lha sorts accents?

@quazardous
Copy link
Contributor Author

capture d ecran de 2017-01-10 12-02-19

@coderaiser
Copy link
Owner

OK, so ls sorts accents correctly, but in Cloud Commander you will see wrong sorting. Could you please make a pull request with just a fix of current readify sort with localeCompare and link to article in comments, while we working on this.

@quazardous quazardous mentioned this pull request Jan 10, 2017
@coveralls
Copy link

coveralls commented Jan 10, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 7f11afd on quazardous:master into 4d97dd0 on coderaiser:master.

# Conflicts:
#	.gitignore
#	lib/readify.js
#	test/readify.js
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.167% when pulling 3a6b4aa on quazardous:master into 1a527be on coderaiser:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.167% when pulling 3a6b4aa on quazardous:master into 1a527be on coderaiser:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.167% when pulling 85970c2 on quazardous:master into 1a527be on coderaiser:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.167% when pulling 85970c2 on quazardous:master into 1a527be on coderaiser:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73e6639 on quazardous:master into b950572 on coderaiser:master.

@quazardous quazardous closed this Jan 10, 2017
@quazardous quazardous reopened this Jan 10, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73e6639 on quazardous:master into b950572 on coderaiser:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73e6639 on quazardous:master into b950572 on coderaiser:master.

@coderaiser
Copy link
Owner

coderaiser commented Jan 11, 2017

We could add a formatting dedicated function ( in the squad stack line 202 ) to avoid having the raw attribute condition and manipulate only raw items internally.
The last function would be a formatOutput ish function.

It is a good idea.
I just split up reading and formatting to 2 files: readify and readdir, and changed type argument to options.

Could you please squash your commits to one, remove localeCompare and synchronise with current version of readify.

@quazardous
Copy link
Contributor Author

quazardous commented Jan 11, 2017

so I have to sort in the readdir file ?
there is just one flaw about owner sorting: it will be sorted by uid not by owner name...
owner mapping could be put in readdir or drop support of owner sorting.

@coderaiser
Copy link
Owner

coderaiser commented Jan 12, 2017

so I have to sort in the readdir file ?

readdir only for reading directory content, all sorting and format in readify.

there is just one flaw about owner sorting: it will be sorted by uid not by owner name...
owner mapping could be put in readdir or drop support of owner sorting.

We can start from adding sorting by size, name, and modification time with order support. And drop support of owner sorting. We can add it later.

@quazardous
Copy link
Contributor Author

OK you've just move sorting from readdir to readify :p because as it, I was confused.
OK I'll do it ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants