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

stat files before serveIndex, also output date, size, and type #74

Closed
wants to merge 7 commits into from

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Aug 13, 2018

As discussed in #73 this PR moves the stating of files into serveIndex so that serveIndex.plain, serveIndex.json, and serveIndex.html consistently output the same data

This does not yet include the updates to the tests. Tests are passing.

Also addresses #39

index.js Outdated
var type = accept.type(mediaTypes);

// not acceptable
if (!type) return next(createError(406));
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to break using this module above your other handlers. This will block all request that doesn't have the right Accept header even if this module is just going to do nothing because the folder doesn't exist.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense.

I originally moved it up so that it would be checked before all of the fs.stat()s. I moved it just after the directory stat and before the fs.readdir.

index.js Outdated
send(res, 'text/plain', (files.join('\n') + '\n'))
serveIndex.plain = function _plain(req, res, dir, files) {
var directory = {
name: dir.name.replace(/\/$/, '\/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this replace actually do anything? I can't figure out what it does, can you ellaborate?

Copy link
Author

Choose a reason for hiding this comment

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

change to dir.name.replace(/\/?$/, '/')
/ => /
demo => demo/

index.js Outdated
*/

var SIZES = [ 'B', 'K', 'M', 'G', 'T', 'P', 'E' ]
function hsize(size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a module here instead of adding this code to the module. The bytes module is one I know, but you're welcome to use a different one if there is one you like better.

Copy link
Author

Choose a reason for hiding this comment

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

That makes me sad. There's so many packages in node that do next to nothing and somehow end up including 100's of megabytes of download. The bytes package is WAY overkill for something this simple.

That said, I'll make the change.

Copy link
Author

Choose a reason for hiding this comment

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

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be a diff package if there is a better one like I said. The other alternative is that this would need tests to cover the conversations (for instance I deleted the "E" from the list and no tests seemed to catch that -- I also changed 1024 to 999 and nothing failed) and it creates more things I have to maintain in the long term, which is harder when it doesn't have tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

(especially considering the recent fiasco with eslint - who knows what percentage of npm was compromised from that alone)

Ok, but why would it be any different that this module itself cannot be compromised?

@coolaj86 coolaj86 changed the title #73 stat files before serveIndex, also output date, size, and type stat files before serveIndex, also output date, size, and type Aug 13, 2018
@dougwilson
Copy link
Contributor

Just as a general comment, this pr is doing many changes all at once. That's fine to review up front, but it'll need to be split into discrete changes prior to eventually landing. The standard practice is that each line in the history file describes a discrete change which in turn is one commit. You can alternatively keep each discrete change organized in their own commits in the single pr and it can be rebase merged instead of the normal squash merged.

This is just a note now so it's not a surprise later when it because difficult to split apart.

@coolaj86
Copy link
Author

In my mind all of the changes are tied together. Moving where the stat() is done changes the API significantly. I don't believe I've made any changes in this string of commits that are not directly related to that.

Do you just want me to rebase this with a single comment like "expose fs.stat() to plain, json, and html templates"?

Or do you want this broken down in some other way?

@dougwilson
Copy link
Contributor

The showing the file size in human format instead of bytes is clearly unrelated to me, if that helps as an example.

@coolaj86
Copy link
Author

coolaj86 commented Aug 13, 2018

@dougwilson Before it didn't show size at all. Now that it shows size, it must show it in some way.

Before when it didn't show bytes, the files were aligned and easy to read.

To show it in raw bytes would break the formatting, which would be a bigger change (in my mind) than the alternative.

In my mind I've done the absolute minimum to expose the data in the most standard way with the most minimal change to the overall experience of anyone using it presently (despite the breaking change).

Can you give me a breakdown of what you'd like and I'll redo it whichever way?

@dougwilson
Copy link
Contributor

Though I am on my phone so hard to take in the entire changeset, it seems to break apart into separate commits like the following to me:

  1. Provide full file into to formatter functions
  2. Change html file size to human format
  3. Display addition file information in json format
  4. Display additional file information in text format

I hope that help :)

@coolaj86
Copy link
Author

html file size was not changed

@coolaj86
Copy link
Author

Got it. 1 big step forward, 3 steps back, then 3 individual steps forward.

@dougwilson
Copy link
Contributor

Sorry, as I said I'm on my phone and just guessing to help provide an example :) I just assume you made them a display the same way irt human sizes, my bad.

@dougwilson
Copy link
Contributor

I'm happy to help guide you with large changes to match the existing practices, but you're bot asking how to do anything first, just doing it. If you don't want to run i to issues, perhaps we should stop and get together and discuss so we're on the same page? Not sure if you know another way. I'm trying to be helpful but comments like that come off kind of rude.

@coolaj86
Copy link
Author

coolaj86 commented Aug 13, 2018

Well, I made another two PRs, much smaller. I'll probably open a few more, but feel free to ignore them until you're happy with the first two.

I thought I had asked the questions that were important to ask in the previous issue. I prefer to just do stuff and talk about it later - because then there's something concrete and tangible to talk about. I feel like code communicates more effectively than English does.

The only pet peeve I have is when things get to a point of bickering about how to name variables. I'm happy to do 95% of the work, but when it comes to itsy bitsy details of personal preference I expect maintainers to click the edit button themselves at that point - I'm a human, not a Siri for dictating and retyping codestyle endlessly.

That said, seeing as how this codebase is fairly inconsistent and has multiple authors using the same variable names in different contexts to mean different things, I don't expect that to be a problem here.

@dougwilson
Copy link
Contributor

I'm not sure if you are confusing me for someone else here. I have not made any kind of comment along the lines of what the variables should be named as.

@coolaj86
Copy link
Author

Rereading my comment...

You had said that I’m not asking, I’m just doing.

I was saying that it’s easier for me to just do and then talk (because then the communication is 100%), I don’t mind reworking something - except if it comes to the point of dictating down to the names of variables. I wasn’t accusing you of doing that.

However, it is quite common on GitHub for people to spend more time critiquing minutia than it would have taken them to simply make the change.

Sorry for the confusion.

@coolaj86
Copy link
Author

@dougwilson Just wanted to make sure you saw my comment above: I was not accusing you.

I was saying that I don't mind doing a number of PRs and getting feedback - because I prefer to communicate in code rather than English for implementation details - as long as it doesn't get down to mundane minutia like variable naming.

I made a flurry of PRs and was expecting to have some more back and forth so that you can see exactly what my intent is, without any ambiguity, and that I can get your feedback on that.

@dougwilson
Copy link
Contributor

Yes, I did read your comment :) I have just been away (my github activity reflects that as well). I will be back soon and will take a look, though there was a security issue reported while I was away, which may take some of my attention at first.

@coolaj86
Copy link
Author

@dougwilson I'd still be interested in getting some of these commits through.

My ultimate goal is to make it easy to have arbitrary user templates (using a variation of the existing template as a default).

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

Successfully merging this pull request may close these issues.

2 participants