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

Faulty times #195

Open
wants to merge 6 commits into
base: development
Choose a base branch
from
Open

Faulty times #195

wants to merge 6 commits into from

Conversation

Eb-Zeero
Copy link
Collaborator

@Eb-Zeero Eb-Zeero commented Apr 29, 2022

This change is Reviewable

adjust the display
remove all histograms with wrong requested times
Copy link
Contributor

@hettlage hettlage left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Eb-Zeero)


src/api/graphQL.js line 431 at r1 (raw file):

  .then(
    response => response.data.data.statistics
  ).catch(() => {})

That looks dangerous to me. You should log the error.


src/api/graphQL.js line 484 at r1 (raw file):

    .then(
      response => response.data.data.proposals
    ).catch(() => {})

That looks dangerous to me. You should log the error.


src/components/tables/statisticsTables/ObservingStatisticsSeeing.js line 9 at r1 (raw file):

  const { numberOfProposals, timeRequested } = seeingDistribution
  const totalReqTime = timeRequested.lessEqual1Dot5 + timeRequested.lessEqual2 +
    timeRequested.lessEqual2Dot5 + timeRequested.lessEqual3 + timeRequested.moreThan3

This line might need discussion.

Copy link
Collaborator Author

@Eb-Zeero Eb-Zeero left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hettlage)


src/api/graphQL.js line 431 at r1 (raw file):

Previously, hettlage wrote…

That looks dangerous to me. You should log the error.

Now logging


src/api/graphQL.js line 484 at r1 (raw file):

Previously, hettlage wrote…

That looks dangerous to me. You should log the error.

Now logging


src/components/tables/statisticsTables/ObservingStatisticsSeeing.js line 9 at r1 (raw file):

Previously, hettlage wrote…

This line might need discussion.

Fixed

give variable more meaning full names
Copy link
Contributor

@hettlage hettlage left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Eb-Zeero)


src/index.js line 29 at r2 (raw file):

  composeWithDevTools(applyMiddleware(thunk))
)
console.log("Mode: ", process.env.NODE_ENV)

Do you need this log statement?


src/components/tables/statisticsTables/ObservingStatisticsSeeing.js line 24 at r2 (raw file):

        <tbody>
          <tr>
            <td>0 &le; Max Seeing &le; 1.5 </td>

Some of the <= signs need to be < signs...


src/util/index.js line 478 at r2 (raw file):

export const logStacktrace = (e) => {
  if (process.env.NODE_ENV === "development") {

This means you wouldn't log the error to Sentry in production, I think.

Copy link
Contributor

@hettlage hettlage left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Eb-Zeero)

Copy link
Collaborator Author

@Eb-Zeero Eb-Zeero left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hettlage)


src/index.js line 29 at r2 (raw file):

Previously, hettlage wrote…

Do you need this log statement?

removed


src/components/tables/statisticsTables/ObservingStatisticsSeeing.js line 24 at r2 (raw file):

Previously, hettlage wrote…

Some of the <= signs need to be < signs...

fixed problem somewhere


src/util/index.js line 478 at r2 (raw file):

Previously, hettlage wrote…

This means you wouldn't log the error to Sentry in production, I think.

Done.

Copy link
Contributor

@hettlage hettlage left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Eb-Zeero)

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

Successfully merging this pull request may close these issues.

2 participants