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

http Fix typo in util functions #888

Merged
merged 5 commits into from
Jan 9, 2025
Merged

http Fix typo in util functions #888

merged 5 commits into from
Jan 9, 2025

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Jan 9, 2025

Edited

Fix typo in jsdoc for util functions and remove export for addAuth()

OLD SUMMARY

Summary

I have separated helpers that are used in Adaptor.js from util functions which are exported in index.js.

  • Fix typo in jsdoc for util functions
  • Move helper functions to helper.js
  • Remove export for addAuth() helper

This PR is addition to #865

Details

The current implementation exports all util functions which is wrong because we shouldn't have access to util.request(), util.addAuth() or util.xmlParser(). These are helpers intended to be used in Adaptor.js functions. I have separated helpers function from util function so that we only export encode, decode, uuid from util.js and use the helpers only in Adaptor.js file

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

@mtuchi mtuchi changed the title re-organize helpers and util http separate helpers from util functions Jan 9, 2025
@mtuchi mtuchi marked this pull request as ready for review January 9, 2025 10:11
@mtuchi mtuchi requested a review from josephjclark January 9, 2025 10:11
@mtuchi mtuchi mentioned this pull request Jan 9, 2025
7 tasks
@@ -148,7 +8,7 @@ export {
* @param {string} data - The string to be encoded.
* @returns {string} - The Base64 encoded string.
* @example <caption>Encode a string</caption>
* const encoded = Util.encode('Hello World');
* const encoded = util.encode('Hello World');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch!

@josephjclark
Copy link
Collaborator

@mtuchi I'm not so sure about this

I don't really like helpers.js as a file name - it doesn't mean anything.

The internal helpers like addAuth are not documented publicly. They are technically available on the utils object, it's true, but they're easily ignored by job code and docs.

I don't really mind mixing public and private functions in js files. We do it in Adaptor.js anyway and I have no problem doing it in utils.js either. And besides, does it really matter if a util function is aimed at job code or other adaptors? Aside from documentation I don't think it does.

@mtuchi
Copy link
Collaborator Author

mtuchi commented Jan 9, 2025

Fair point. I wish there was a better way of doing this because if i add documentation to any function in util i have to add @private if i don't that function to appear but still i will be able to use it in my job code.

Write now even though util.request() does not show up in docs, i can still use it in my job code
Screenshot 2025-01-09 at 8 36 52 PM

Let me revert and fix the typo only for this PR so that we can get #865 released

@mtuchi mtuchi requested a review from josephjclark January 9, 2025 17:43
@mtuchi mtuchi changed the title http separate helpers from util functions http Fix typo in util functions Jan 9, 2025
@josephjclark
Copy link
Collaborator

I wish there was a better way of doing this because if i add documentation to any function in util i have to add @Private if i don't that function to appear but still i will be able to use it in my job code.

Wait are you sure?

My understanding (and I've written a lot of code around this stuff, but I can definitely be wrong) is that a function needs @public and @function before it's documented.

Because we DO use private internal functions. So I think things should be private by default, and only made public when someone's made the effort to properly document them.

I'll have to take a look at this to be certain before moving on.

@josephjclark
Copy link
Collaborator

The docgen in tools says this:

// Filter items which are not marked as @public
  templateData = templateData.filter(
    data => data.kind === 'typedef' || data.access === 'public'
  );

Let me assert that access is private by default though

@josephjclark
Copy link
Collaborator

Yeah, confirmed: common.alterState is not currently documented:

image

It has no explicit access (so it's private by default)

/**
 * alias for "fn()"
 * @function
 * @param {Function} func is the function
 * @returns {Operation}
 */
export function alterState(func) {
  return fn(func);
}

@josephjclark josephjclark merged commit 7ca57d3 into common-helpers Jan 9, 2025
2 checks passed
@josephjclark josephjclark deleted the http-utils branch January 9, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants