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.

Sign up for GitHub

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

Jump to bottom

include dsl tag for all json keys #812

Merged
merged 6 commits into from
Oct 20, 2022
Merged

include dsl tag for all json keys #812

merged 6 commits into from
Oct 20, 2022

Conversation

LuitelSamikshya
Copy link
Contributor

No description provided.

@LuitelSamikshya LuitelSamikshya self-assigned this Oct 13, 2022
@LuitelSamikshya LuitelSamikshya added the Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. label Oct 13, 2022
@LuitelSamikshya LuitelSamikshya linked an issue Oct 13, 2022 that may be closed by this pull request
Include all the JSON output keys #797
Closed
@Mzack9999
Copy link
Member

@ehsandeep shall we keep the dsl specific tag or infer the dsl list directly from the json tag since the name is supposed to be the same?

@ehsandeep
Copy link
Member

infer the dsl list directly from the json tag since the name is supposed to be the same.

@Mzack9999 this, since the name is supposed to be the same, we can directly list all the existing keys from json instead of maintaining them separately.

Mzack9999
Mzack9999 requested changes Oct 15, 2022
View reviewed changes
Copy link
Member

@Mzack9999 Mzack9999 left a comment •
edited by LuitelSamikshya

Choose a reason for hiding this comment

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

As per @ehsandeep confirmation, the list should be inferred from the json tag:

  • Remove dsl tag
  • Build the list using json tag and its attributes

Mzack9999
Mzack9999 requested changes Oct 18, 2022
View reviewed changes
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

Proposed a different approach

runner/types.go Outdated Show resolved Hide resolved
runner/types.go Outdated Show resolved Hide resolved
runner/types.go Outdated Show resolved Hide resolved
Mzack9999
Mzack9999 previously requested changes Oct 18, 2022
View reviewed changes
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

suggesting different approach

@Mzack9999
refactoring expression engine
857ab37 
- Using mapstructure for uniform mapping
- Flat key:value in expressions/variables list
@Mzack9999 Mzack9999 self-assigned this Oct 20, 2022
@Mzack9999 Mzack9999 dismissed their stale review October 20, 2022 06:41

refactored expression engine

ehsandeep
ehsandeep previously requested changes Oct 20, 2022
View reviewed changes
Copy link
Member

@ehsandeep ehsandeep left a comment

Choose a reason for hiding this comment

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

./httpx -ldv

[FTL] interface{} not allowed

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ehsandeep ehsandeep merged commit 12fee4f into dev Oct 20, 2022
@ehsandeep ehsandeep deleted the issue-797/include-all-json-keys branch October 20, 2022 07:48
@Mzack9999 Mzack9999 mentioned this pull request Oct 23, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehsandeep ehsandeep ehsandeep approved these changes

@Mzack9999 Mzack9999 Mzack9999 approved these changes

Assignees

@Mzack9999 Mzack9999

@LuitelSamikshya LuitelSamikshya

Labels
Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include all the JSON output keys
3 participants
@LuitelSamikshya @Mzack9999 @ehsandeep