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

adds logic for gcn.notices. notices, replace healpix_file if present #162

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

dakota002
Copy link
Collaborator

No description provided.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

  • Replace all fields larger than a certain size, not just the healpix_file field.
  • Fix CI failures.

@dakota002
Copy link
Collaborator Author

  • Replace all fields larger than a certain size, not just the healpix_file field.
  • Fix CI failures.

For the first point, is there a size you have in mind? Like are we talking a max string length?

@lpsinger
Copy link
Member

For the first point, is there a size you have in mind? Like are we talking a max string length?

Yes.

@dakota002
Copy link
Collaborator Author

For the first point, is there a size you have in mind? Like are we talking a max string length?

Yes.

Any rough idea what the cutoff should be? 256, 1000, ?

@lpsinger
Copy link
Member

Any rough idea what the cutoff should be? 256, 1000, ?

Let's start with 512 bytes.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

You need to traverse the entire JSON object, not just the top-level dictionary entries.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

There are a lot of unrelated, non-functional, stylistic changes here. It looks like you ran a code formatter on this. Please do that in a separate PR that contains only the reformatting.

@dakota002
Copy link
Collaborator Author

What are the linting settings for this project? My vscode keeps replacing stuff on save that I don't intend it to

@lpsinger
Copy link
Member

What are the linting settings for this project? My vscode keeps replacing stuff on save that I don't intend it to

We don't have any. Tell you what, I'll make a PR to set up black.

gcn_email/core.py Outdated Show resolved Hide resolved
Name='/RemixGcnProduction/tables/email_notification_subscription'
)
Copy link
Member

Choose a reason for hiding this comment

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

Undo non-functional formatting change.

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

Please rebase to pick up black formatting.

dakota002 and others added 3 commits January 31, 2024 10:42
Fixes formating and check size of all fields

Adds function to traverse dict and replace content thats too long
Co-authored-by: Leo Singer <[email protected]>
@dakota002
Copy link
Collaborator Author

Some of my changes got removed in the rebase, fixing now

@lpsinger lpsinger merged commit d9abf9c into nasa-gcn:main Jan 31, 2024
2 checks passed
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