Skip to content

Conversation

@connorjclark
Copy link

… exports

Issue #15

I wasn't sure of the best interface to provide for getting the safe/idempotent properties. I decided to provide functions 'isSafe' and 'isIdempotent' on the exports. That way, the only change to existing code using this module would be var methods = require('methods) -> var methods = require('methods),methods

I made a dev script to scrape the data from IANA, and store it in a json file. Would you prefer this to be done live instead?

@dougwilson
Copy link
Contributor

You may want to check first before working on something if it helps. There is already an iana branch with the WIP on this. Please base work on that branch, not master.

@dougwilson dougwilson self-assigned this Apr 30, 2017
@dougwilson dougwilson added the pr label Apr 30, 2017
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Work for pulling from IANA (issue #15) needs to be based on the iana branch; adding other exports needs to be a separate PR from the IANA pulling work.

@connorjclark
Copy link
Author

I'm new to making contributions on GitHub, didn't think to look at branches, obvious in hindsight. Will keep that in mind for the future.

Looks like you did the same thing I did but also wrote your own JSON serializer. Just curious, was that for fun, or is there a particular benefit?

If it sounds good to you, I will branch properly and add the tests I wrote / change the exports in another PR.

@dougwilson
Copy link
Contributor

I'm new to making contributions on GitHub, didn't think to look at branches, obvious in hindsight. Will keep that in mind for the future.

That's OK; I'm happy to help if you wanted to help on an issue I had assigned to myself to do, but you may want to comment in the issue in the future to see how you can help / where to start :)

Looks like you did the same thing I did but also wrote your own JSON serializer. Just curious, was that for fun, or is there a particular benefit?

It's the same pattern I use in the other repos; it ensures that the JSON object keys are always in the same order and don't switch around sometimes by running a .sort() on them (which JSON.seralize does not do), which is important when it's getting checked in. On other modules pulling from IANA the file kept churning until that change was made (making giant, unreadable diffs).

YI had just made #15 just as a reminder for me, so didn't write down any details. Pulling down from IANA is intended to completely change what this module is and how exactly the Node.js fallback is supposed to work, which is part of the pull from IANA to make useful work what is the missing part from that branch. The idea is that this module should only provide the methods from IANA and not use that hard-coded list at all.

@connorjclark
Copy link
Author

That's OK; I'm happy to help if you wanted to help on an issue I had assigned to myself to do, but you may want to comment in the issue in the future to see how you can help / where to start :)

Will do! :)

It's the same pattern I use in the other repos; it ensures that the JSON object keys are always in the same order and don't switch around sometimes by running a .sort() on them (which JSON.seralize does not do), which is important when it's getting checked in. On other modules pulling from IANA the file kept churning until that change was made (making giant, unreadable diffs).

Interesting. Those sort of diffs would be super annoying.

YI had just made #15 just as a reminder for me, so didn't write down any details. Pulling down from IANA is intended to completely change what this module is and how exactly the Node.js fallback is supposed to work, which is part of the pull from IANA to make useful work what is the missing part from that branch. The idea is that this module should only provide the methods from IANA and not use that hard-coded list at all.

I suppose I don't understand why you would need any fallback at all with this change, given that we have all the methods from IANA in a json file.

If IANA gives a method not found in http.METHODS, I assume Node.js would not correctly handle that method. In that case, should this module not return any information about that method (or perhaps that's not worth any consideration...) ?

Something like:

module.exports = loadMethodsFetchedFromIANA().filterOutThoseNotInHTTPModule()
/*
{
  "acl": {
    "idempotent": true,
    "safe": false,
    "sources": [
      "https://tools.ietf.org/html/rfc3744#section-8.1"
    ]
  },
  "baseline-control": {
    "idempotent": true,
    "safe": false,
    "sources": [
      "https://tools.ietf.org/html/rfc3253#section-12.6"
    ]
  },
  ...
}
*/

Is the above pseudocode missing any features you want out of this module?

Of course, the above wouldn't work if http.METHODS isn't available.

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.

2 participants