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

Support Options to useOnlyUTC in the mail header #2

Open
wants to merge 2 commits into
base: random-fixes
Choose a base branch
from

Conversation

viggyprabhu
Copy link

This PR supports all changes needed to support useOnlyUTC option in jsmime Header. This way, we can now customize jsmime to only use UTC as its TimeZone in the mail header so that the user's location through the TimeZone info is not leaked through the mail header.

@viggyprabhu
Copy link
Author

The PR is generated against random-fixes branch as that is supposed to have the latest code.

@viggyprabhu
Copy link
Author

@jcranmer Any comments on how I can improve this PR?

@@ -72,10 +72,17 @@ function clamp(value, min, max, def) {
* @param [options.useASCII=true] {Boolean}
* If true, then RFC 2047 and RFC 2231 encoding of headers will be performed
* as needed to retain headers as ASCII.
* @param [options.useOnlyUTC=true] {Boolean}
* If true, then date added in header is always in UTC. This is to provide
* user location privacy by not revealing Timezone location.
Copy link
Owner

Choose a reason for hiding this comment

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

The option is probably better named "hideDateTimezone". Also, your English is not grammatically correct here:

If true, then use the UTC time zone instead of the local time zone when emitting date values.

[No need to mention the second sentence]

Also, note that "time zone" is two words in English, not one, and that capitalization is reserved for acronyms or the first word of a sentence.

@jcranmer
Copy link
Owner

Some more general comments:

  • This should be compressed into one commit, not two.
  • There is a lot of random extra trailing whitespace. Make it go away.
  • I'm picky about style. So try to make sure that you're following 80-char limits, and make sure that you insert the proper number of spaces in places: I don't like if (foo){ but instead want to see the text if (foo) {. Look around the file if you're confused about the style.
  • Also, try to write proper English sentences in comments, complete with punctuation.

@viggyprabhu
Copy link
Author

Thanks for all the feedback, I will fix the issues and incorporate the
changes and send a new PR.

On Saturday 26 September 2015 12:15 PM, Joshua Cranmer wrote:

Some more general comments:

  • This should be compressed into one commit, not two.
  • There is a lot of random extra trailing whitespace. Make it go away.
  • I'm picky about style. So try to make sure that you're following
    80-char limits, and make sure that you insert the proper number of
    spaces in places: I don't like |if (foo){| but instead want to see
    the text |if (foo) {|. Look around the file if you're confused about
    the style.
  • Also, try to write proper English sentences in comments, complete
    with punctuation.


Reply to this email directly or view it on GitHub
#2 (comment).

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