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

feat: Add optional file flags for cli view add command #3314

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

najeal
Copy link

@najeal najeal commented Dec 11, 2024

Relevant issue(s)

Resolves #3280

Description

This PR adds add optional way to provide query and sdl data from a file when using cli view add command.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

It has been tested adding new unit tests.

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle
Copy link
Collaborator

Hi @najeal. Thank you for contributing this PR.

The pattern in your implementation is clever but makes it hard for users to understand easily how to use the command as mixes ordered args and unordered flags.

I would recommend switching entirely to flags like we do with some of our other commands. Cobra has some functionality to do an exclusif or on a set of flags so we could easily do.

--query xor --query-file
--sdl xor --sdl-file
--lens xor --lens-file

Once/if you decide to do the change, please update the command usage description accordingly :)

@najeal
Copy link
Author

najeal commented Dec 12, 2024

Hi @fredcarle ! I will do the changes 👍

@najeal
Copy link
Author

najeal commented Dec 12, 2024

Changes are done 👍

@fredcarle fredcarle force-pushed the najeal/feat/cli-view-add-optional-file branch from 3a91ed6 to e3a3bd3 Compare February 11, 2025 16:18
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @najeal! Sorry for the delay in approving this. We've been at work on other priorities. I'll be merging your PR once the CI completes its required workflows.

@fredcarle fredcarle changed the title feat: add optional file flags for cli view add command feat: Add optional file flags for cli view add command Feb 11, 2025
@najeal
Copy link
Author

najeal commented Feb 11, 2025

@fredcarle no problem ! If you see any good first issue I could look, let me know !

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 48.83721% with 22 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (58bc5ee) to head (e3a3bd3).

Files with missing lines Patch % Lines
cli/view_add.go 48.84% 22 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3314      +/-   ##
===========================================
- Coverage    78.47%   77.18%   -1.29%     
===========================================
  Files          396      396              
  Lines        37333    37355      +22     
===========================================
- Hits         29295    28830     -465     
- Misses        6352     6857     +505     
+ Partials      1686     1668      -18     
Flag Coverage Δ
all-tests 77.18% <48.84%> (-1.29%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cli/view_add.go 38.36% <48.84%> (-32.23%) ⬇️

... and 53 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58bc5ee...e3a3bd3. Read the comment docs.

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

The CI raised an issue with integration tests using the CLI as a client. Could you please update tests/clients/cli/wrapper.go#wrapper.AddView to fit the changes that you made.

You can run the following command to see the result of one of the failing tests locally. They all have the same issue so fixing that one should fix the others too.

DEFRA_CLIENT_CLI=true go test -run ^TestView_Simple_GQLIntrospectionTest$ ./tests/integration/view/simple

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.

Add file options to View CLI add command
2 participants