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

added metadata view in mitm\web #5983

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Rishabhg71
Copy link

@Rishabhg71 Rishabhg71 commented Mar 10, 2023

Description

closes #5704

  • Added metadata viewer in mitm\web.

Checklist

  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

I don't think any test is required in this case

@Prinzhorn
Copy link
Member

I assume you or the editor you're using were doing some automatic code formatting. This makes it hard to focus on the actual changes. We should definitely clean this up at some point (e.g. mixed single/double quotes or missing spaces) but it would be better not to have these changes in this PR.

@Rishabhg71
Copy link
Author

I tried to have the proper formatting for web but there is no formatter config available
Maybe in the web we can use prettier for web

@Prinzhorn
Copy link
Member

I've opened #5984 to fix this in the future. For now I recommend disabling any formatting your editor does. That's a recommendation I would give in general when contributing to a project. Formatting changes make PRs unnecessarily noisy. Unrelated code should not be touched.

@Rishabhg71
Copy link
Author

Ok i will submit make the changes and push it

@Rishabhg71
Copy link
Author

ignore: to make it easy to compare changes
made this commit to revert the changes to original formatting

changes made in mitm web
made the changes without formatting

@Prinzhorn
Copy link
Member

Perfect, I think that's good for a first iteration. Two small changes:

  1. We should allow any type of metadata, so I'd check for null (None in Python) and throw everything else into JSON.stringify. Fun fact, strings are already rendered because Object.keys('foo') is [ '0', '1', '2' ]
  2. Indent the JSON, e.g. JSON.stringify(flow.metadata, null, 4)

@Rishabhg71
Copy link
Author

Rishabhg71 commented Mar 13, 2023

@Prinzhorn
i have messed up my fork by clicking on discard button when updating my fork
which closed this PR
I have made the changes you asked in 1st iteration

@Rishabhg71 Rishabhg71 reopened this Mar 13, 2023
@@ -126,7 +126,7 @@ def __init__(
self._backup: Flow | None = None
self.marked: str = ""
self.is_replay: str | None = None
self.metadata: Any = None
Copy link
Author

Choose a reason for hiding this comment

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

if i change this to a lot of test start failing so should a leave metadata as empty dict or it must initialized with None

self.metadata: Any = None

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.

Addon: add the possibility to add text/metadata to a connection
2 participants