doc: add info about DEBUG mode

[gireeshpunathil]

Many issues pertinent to session management
can be easily diagnosed with debug on.
Make a statement to that effect in the README
to help users self-assist to an extent.

Refs: https://github.com/expressjs/session/issues/659

[gireeshpunathil]

bump. Is there anything I can do to help moving this forward? thanks!

[MitchTalmadge]

This would have helped me out.

[jonchurch]

I would love to see this get merged in, @wesleytodd @dougwilson any feedback here?

[dougwilson]

I have had this in a module before and it resulted in endless PRs for al the different methods to set environment variables on the different OSes / shells. I would prefer not to go through that again and end up with a giant list of these :) If it is possible, just state what our namespace is (express-session and point the user to the debug module on how to turn on debugging for a given namespace.

[jonchurch]

Would it suffice to give a command that suggests running your project while setting the DEBUG flag aka DEBUG=express-session node index.js? I'm not 100% but I think that is an appropriate cross platform way to set the ENV var?

[dougwilson]

Sadly that syntax does not work on cmd.exe, nor on PowerShell, both the main Windows shells... :(

[gireeshpunathil]

@dougwilson - my motivation is to document critical, yet missing information that aids self-diagnosing issues of using this module. Reading it multiple times, I don't see any misleading, confusing or redundant phrases, nor any incomplete info that will / may invite future bike-shedding.

I would like to retain the changes. I see you did not use the red button, so I treat your comment as a suggestion that is free to leave out, not a blocker.

[dougwilson]

The issue is in the past, this just leads to endless issues being opened reporting the following error:

[dougwilson]

And yes, you are 100% correct, I did not "request" changes, and only made a comment :) That was intentional... I am not saying there needs to be a change. I was thinking about where we have left something similar and it was http://expressjs.com/en/starter/generator.html … after countless PRs from users "fixing" the DEBUG command. Maybe just copy that structure since we haven't had a PR in a while around that. I think the current structure leads to two issues:

1. users read top-to-bottom, and will try to run a command given before reading down below. Only below it says that doesn't work on Windows. On expressjs.com what is works on is disclosed before the command.
2. The text says "use set in Windows" but I can tell you from experience users … have no idea that that means (this is not unique to windows; export has the same thing). It would seem to either say "use export" and "use set" or give full examples for both.

[gireeshpunathil]

Setting this property prior to the run (use 'set' in Windows) - the first line in my change has this, that should clarify, and work I guess?

[gireeshpunathil]

may be we all 3 talked almost at the same time, so I am not sure about the ordering of the above conversation - who is asking, and who is answering to whom! :)

[gireeshpunathil]

ok , I understand your reasons. Such users (who cannot reasonably follow the document) can also copy-paste the dollar symbol, and app.js from the phrase $ DEBUG=express-session node app.js while there shell prompt and app name might have been something else?

my take would be to balance between readability and usability, while accepting reasonable changes (in the future) and provide explanation to the rest.

[dougwilson]

Right, and that's why I suggest we just copy what is on expressjs.com page I shared, as that seems to have calmed down the PRs. I really don't want to land a PR that is known to create a cascade of more PRs to deal with.

[dougwilson]

Hi @jonchurch sorry I missed this PR. I labeled it up and left a comment 👍

[dougwilson]

Specifically requesting to update the text around the command(s) to enable debugging.

[gireeshpunathil]

Right, and that's why I suggest we just copy what is on expressjs.com page I shared, as that seems to have calmed down the PRs.

@dougwilson - I have to disagree with your reasoning here. presence of a piece of text / code / doc did not invite further PR is not a good reason to infer that that is the text which is better for consumption. Potential reasons for less PRs could be, but not limited to:

• seen less
• less needed to debug

This is a documentation change. IMO its merit is best measured in terms of

• correctness
• usability
• preciseness

It is sad to see you block this PR based on a reason that is outside of these!

[dougwilson]

I am measuring it against what you are saying, for example my point number 2 above:

The text says "use set in Windows" but I can tell you from experience users … have no idea that that means (this is not unique to windows; export has the same thing). It would seem to either say "use export" and "use set" or give full examples for both.

Maybe you can help me understand why should there be an example for the syntax of export but not for the syntax of set?

[gireeshpunathil]

Maybe you can help me understand why should there be an example for the syntax of export but not for the syntax of set?

It is based on the perceived user base: UNIX used more than Windows.

[dougwilson]

Sure, but unless you have some statistics to actually back that statement up, that seems to be argued on feeling vs facts, though :) I actually perceive it to be the exact opposite, having only Windows machines myself and working in a Windows-based development company. I'm not sure if that view is any less valid.

I can certainly agree if you can provide the operating system statistics of the express-session user base if that if what we want to base this on, though.

[gireeshpunathil]

I actually perceive it to be the exact opposite,

ok, I may be wrong, and I don't have statistics.
I bank on your perceptions as the lead maintainer of this project.
Just reversed the example.
PTAL!

[dougwilson]

that's why I suggest we just copy what is on expressjs.com page I shared

[gireeshpunathil]

@dougwilson - why this was closed? You requested changes on this PR, I acted on it, requested for your review 2.5 months back - so at least you owe me an explanation and chance for me to act on it.

Please re-open this PR, and either

• explain why you cannot agree to the changes in the current form or
• approve and progress or
• leave it to other maintainers for their review

[dougwilson]

It was closed from the other commit that did what the change I requested 3 months ago that i quoted above. I assumed that since it had been so long and that change was never made, I would just get it landed while we are trying to cut a new version of this module.

[dougwilson]

Basically I never saw an objection to that request, and not sure what the hold up was, and I remember from the other conversations you would rather I not push into your PRs, so just wanted to wrap this up from what I didn't see objection to. I didn't specifically close the PR, rather GitHub auto closed the PR once a change was merged in that resolve the same contents as this...

[gireeshpunathil]

@dougwilson - you did not follow the community's Contributing Guide while over-riding this with your own commit, namely:

• Encourages new contributions.

• Encourages contributors to remain involved.

• Avoids unnecessary processes and bureaucracy whenever possible.

• Creates a transparent decision making process that makes it clear how contributors can be involved in decision making.

• Any change to resources in this repository must be through pull requests. This applies to all changes to documentation, code, binary files, etc. Even long term committers and TC members must use pull requests.

• No pull request can be merged without being reviewed.

Please revert your change, re-open this PR, and either collaborate on this to progress or remove your red-X

[dougwilson]

Hi @gireeshpunathil I think you need to perhaps take a breath from here and step back for a bit. I''m not sure what your goal here, but I think you may be overstepping quite a bit here, but maybe @expressjs/express-tc would like to chime in...

[wesleytodd]

Hey! As with any team growth we will have bumps! It seems like this is one, and hopefully we can come to a healthy resolution here.

Here is what I think went well above:

1. So having not followed this PR until now, I want to say that I think this is a good addition. More docs, and more clear docs are a huge win for the project.
2. I think there was a very healthy debate on the "why have one example and not the other", this is GREAT, and we all need to get comfortable with this kind of discussions. Neither person is wrong I think and you both stated your cases.
3. I also think the feedback on following the example from the website, which is what landed in 909d9e0 is better that the original version.
4. Lastly, thank you @gireeshpunathil for holding @dougwilson accountable, we as a team need to do this for each other!

Where I think this went wrong:

1. @dougwilson I think it was a bit unclear in your phrasing here: "that's why I suggest we just copy what is on expressjs.com page I shared". I am guessing that this did not come across as a direct review request to @gireeshpunathil?
2. @gireeshpunathil, I think as a team we can have better clarity about what a "request changes" PR review means. I don't think that @dougwilson was intending to "block this PR", just to say that having both enumerated like is on the website is a better way to show it.
3. In an effort to move the code/docs forward @dougwilson pushed the change and closing the issue with another commit can be very unsatisfactory. We all can make decisions which result in accidentally hurting team member, remember to be gentle on both sides. I don't think @dougwilson intended to offend, break trust, or go against the contributor guide and conduct in that action.

What I think we can do from here:

1. All take a deep breath and remember that we are all giving our own free time to make this project better.
2. Next time we have a small (and everyone must agree this is a small) disagreement on some bit of code or docs, start from a place of assuming best intentions and remember that the team is more important than the code.
3. Consider next steps for the PR, I think we have three options, all with pros/cons:

• Force master back to bcf1f07. Have @gireeshpunathil update his examples section to match the way it is in 909d9e0 and re-merge.
• Re-base master so that it is on this commit with the changes from 909d9e0
• Do nothing but learn from our communication miss

I am personally happy with any of the three next steps as long as we all take a step back to reflect on the direction this thread took and commit giving direct and honest feedback and attempt to make more careful decisions in the future.

[gireeshpunathil]

@wesleytodd - thanks. I take your feedback, and promise to gain better clarity on request changes. Regarding the next steps, I am fine with TC's any recommendations, including Do nothing but learn from our communication miss.

@dougwilson - I do not consider your actions (of committing un-reviewed code and closing this PR) were in bad faith. As @wesleytodd said, let us pledge to engage in healthy conversations and work towards convergence, for the interest of the project and community!