Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Non-standard indentation and undefined catch scope #119

Closed
robertosousa1 opened this issue Nov 22, 2020 · 6 comments
Closed

Non-standard indentation and undefined catch scope #119

robertosousa1 opened this issue Nov 22, 2020 · 6 comments
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@robertosousa1
Copy link

In the file "lib/ndjson.js" the tryJSONStringify function is declared below your call, this causes an "Expected to return a value at the end of function 'tryJSONStringify' indentation conflict" and this function still has nothing defined in its catch scope.

I suggest implementing the following code:

'use strict'

const stringify = require('fast-safe-stringify')

function tryJSONStringify (obj) {
  try {
    return JSON.stringify(obj)
  } catch (e) {
    throw new Error('Error when running tryJSONStringify')
  }
}

exports.serialize = function serialize (obj) {
  const str = tryJSONStringify(obj) || stringify(obj)
  return str + '\n'
}

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 22, 2020
@astorm
Copy link
Contributor

astorm commented Nov 23, 2020

@robertosousa1 Thanks for the bug report @robertosousa1 -- when you say you're seeing the error

expected to return a value at the end of function 'tryJSONStringify' indentation conflict"

Is this an error you see while running the agent? Or is this a linting error related to the other issues you've opened?

@K-Kumar-01
Copy link

@astorm
Is the issue still open?
If yes then can I make a pr for the issue?

@astorm
Copy link
Contributor

astorm commented Dec 8, 2020

@K-Kumar-01 We're still waiting on @robertosousa1 -- when he said

expected to return a value at the end of function 'tryJSONStringify' indentation conflict"

it wasn't clear whether this was a linting error or an "execution of code during the agent error".

@k-g-elastic Can you reproduce the error @robertosousa1 mentioned? If so, can you share those reproduction steps?

@robertosousa1
Copy link
Author

Hi @astorm,

It is a linting error.

If you understand that there is a need for adjustment, I can do the PR.

@trentm
Copy link
Member

trentm commented Oct 12, 2021

Hi All,

From what linting tool do you see this linting error? This repo is currently using standard for linting and that passes:

[apm-nodejs-http-client (git:master)]% npx standard

Note that we don't want to change this to throw an error in the catch block. Perhaps explicitly returning undefined in the catch block would pass your linter and be more explicit. I'd welcome a PR doing that.

@trentm
Copy link
Member

trentm commented Aug 3, 2023

In elastic/apm-agent-nodejs#3507 this repo code was moved to https://github.com/elastic/apm-agent-nodejs/tree/main/lib/apm-client/http-apm-client/. I'm closing this issue here. FWIW this code passes the eslint linting we are using. Please feel free to open a PR on the apm-agent-nodejs.git repo if you need some change to satisfy another linter you are using.

@trentm trentm closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

No branches or pull requests

4 participants