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

Backport 5x 2x header parsing 2023-11-05 #407

Closed

Conversation

dodmi
Copy link
Contributor

@dodmi dodmi commented Nov 13, 2023

This pull request is to backport your changes in header parsing.

By the way, eslint does it's job :)

Copy link
Owner

@lieser lieser left a comment

Choose a reason for hiding this comment

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

Nice to hear that ESLint helped already a little.

A note about the git history:
I try to keep the history clean and not to messy.

In this MR you have:

  • 2 usefully commits (a
    • ARH: allow : in non quoted-string properties if relaxed parsing is en…
    • ARH: don't restrict result keyword for unknown methods
  • 1 Fix commit (a fix for one of the useful commit above). Not the nicest things to have, but would still be ok if not to many.
  • 2 Merge commits

I don't want to have the two merge commit from this PR in the history, as this would be messy.
But I'm also hesitant about just squashing all commits into one, (via the GitHub GUI) when I merge this. As the two useful commits would be lost.

I you don't mind the lost history I will probably still just squash it. But I could also rewrite the history for you and clean it up if you want before I merge.

Long term it would probably be good if you look into how rebasing works (see e.g. https://git-scm.com/book/en/v2/Git-Branching-Rebasing). Would have been the better alternative here instead of merging the updated lieser:2.x into your change branch.
Rebasing is maybe a little more complicated, and more things can go wrong compared to merging. But it is a very useful tool to have if you are wiling to go behind the absolute basic git usage.

Comment on lines 18 to +19
/* global Logging */
/* global DKIM_InternalError */
Copy link
Owner

Choose a reason for hiding this comment

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

Note that you can specify multiple variables in one in one of this global statements, Normally I make two groups, one for things coming from Mozilla, and another one the the add-on specific ones.

Suggested change
/* global Logging */
/* global DKIM_InternalError */
/* global Logging, DKIM_InternalError */

But you can keep it as is if you prefer one line for each.

@lieser lieser added this to the 2.3.0 milestone Nov 13, 2023
@dodmi
Copy link
Contributor Author

dodmi commented Nov 14, 2023

I'm closing this pull request and create a new one :)

@dodmi dodmi closed this Nov 14, 2023
@dodmi dodmi deleted the backport-5x-2x-header-parsing-2023-11-05 branch November 14, 2023 21:18
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