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

Format json option #783

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

Format json option #783

wants to merge 16 commits into from

Conversation

MartinFillon
Copy link
Contributor

No description provided.

@MartinFillon MartinFillon linked an issue Jan 16, 2024 that may be closed by this pull request
feat: add json formatted output option #768
Open
@MartinFillon MartinFillon mentioned this pull request Jan 16, 2024
Open
@heuristicAL heuristicAL mentioned this pull request Jan 17, 2024
Closed
@MartinFillon MartinFillon force-pushed the format-json-option branch 2 times, most recently from 6e68614 to 3c4513c Compare February 2, 2024 12:16
@MartinFillon
Copy link
Contributor Author

This can either wait for tree tbd or be already merged

@PThorpe92
Copy link
Member

PThorpe92 commented Feb 2, 2024 •
edited
Loading

This can either wait for tree tbd or be already merged

I have a fix incoming, i know it's going to get piped/formatted most of the time but its just changing this
image
to this
image

@PThorpe92
Copy link
Member

Hey I don't think it still actually equivalent to --long view?

I believe now it requires the flag so we might want to change the docs/completions

@MartinFillon
Copy link
Contributor Author

Hey I don't think it still actually equivalent to --long view?

it is as it just takes every cell used by long view and display its content differently

@MartinFillon
Copy link
Contributor Author

I have a fix incoming, i know it's going to get piped/formatted most of the time but its just changing this

tbh only tested with jq '.'

@S1ngularity96
Copy link

I'd like to join the conversation for testing and contributing. I tried the cargo run -- --long --json command. However the output results in an object, where the key files consists of an array of arrays. Is that the right behaviour? Don't we want to have an array with objects that contain keys for each file attribute?

S1ngularity96
S1ngularity96 reviewed Mar 7, 2024
View reviewed changes
src/output/mod.rs
Comment on lines +42 to +46
pub enum OutputType {
Legacy,
Json,
}
Copy link

Choose a reason for hiding this comment

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

I wouldn't call the default output type Legacy. This should be something like Console or TTY?

completions/zsh/_eza
Comment on lines 70 to +72
--smart-group"[Only show group if it has a different name from owner]" \
--stdin"[When piping to eza. Read file names from stdin]"
--json"[Output results as JSON(equivalent to --long)]"
Copy link

Choose a reason for hiding this comment

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

This will fail. You forgot the \ character.

Suggested change
--smart-group"[Only show group if it has a different name from owner]" \
--stdin"[When piping to eza. Read file names from stdin]"
--json"[Output results as JSON(equivalent to --long)]"
--smart-group"[Only show group if it has a different name from owner]" \
--stdin"[When piping to eza. Read file names from stdin]" \
--json"[Output results as JSON(equivalent to --long)]"

@S1ngularity96
Copy link

I don't like the approach here. I would have changed the codebase so that at the end there would always be a collection of structs with the respective file attributes. An output-specific renderer would then output the content. The code keeps getting bloated if more formats are added. There should be one source file for each format in which the rendering strategy is implemented. The render function always gets the same object so that the interface fits. If possible, other source files should not be changed at all.

Maybe something like this.

image

@MartinFillon
Copy link
Contributor Author

I'd like to join the conversation for testing and contributing. I tried the cargo run -- --long --json command. However the output results in an object, where the key files consists of an array of arrays. Is that the right behaviour? Don't we want to have an array with objects that contain keys for each file attribute?

As per the precedent conversations, et to be consistent with current codebase this behaviour needs the --header

Maybe something like this.

I Like the idea you proposing, but that is a bit of work tbh, tho, if you take a look at the current way of doing it, this is already like that as per how I implemented it, its just kinda hidden within the current structs. But refactoring everything is something I am thinking about.

@S1ngularity96
Copy link

I'd like to join the conversation for testing and contributing. I tried the cargo run -- --long --json command. However the output results in an object, where the key files consists of an array of arrays. Is that the right behaviour? Don't we want to have an array with objects that contain keys for each file attribute?

As per the precedent conversations, et to be consistent with current codebase this behaviour needs the --header

Maybe something like this.

I Like the idea you proposing, but that is a bit of work tbh, tho, if you take a look at the current way of doing it, this is already like that as per how I implemented it, its just kinda hidden within the current structs. But refactoring everything is something I am thinking about.

In the future, whenever a new format is added, it will be necessary to refactor it anyway. It would also be better to cleanly separate the data from the rendering logic.
However I also looked through the 'Display Options' again. Tree, Grid and are all separate formats. It somehow doesn't make sense that it's possible to run --tree --grid in one command.

@MartinFillon
Copy link
Contributor Author

Well you got a point on the refactor part, as per the options one, there is priority made, but you can use the envvars to be more precise.

@S1ngularity96
Copy link

If you like I could try to reorganize the code and do a pull request. But only if that's okay and its alright for you to change some underlying architecture of the code.

@MartinFillon
Copy link
Contributor Author

If you like I could try to reorganize the code and do a pull request. But only if that's okay and its alright for you to change some underlying architecture of the code.

We always up for any contribution go ahead as you wish.

@MartinFillon
Copy link
Contributor Author

fixes #472

@MartinFillon MartinFillon force-pushed the format-json-option branch 2 times, most recently from 80f7a2d to e84c470 Compare March 18, 2024 08:39
@MartinFillon MartinFillon self-assigned this May 31, 2024
@MartinFillon MartinFillon added enhancement New feature or request features › ui Arguments Relates to arguments parsing/modifying/Adding labels May 31, 2024
@MartinFillon
Copy link
Contributor Author

Okay @cafkafk and @PThorpe92 this is finally ready for merging

@lespea
Copy link

lespea commented May 31, 2024

Since this is hand rolling the json impl it isn't properly escaping invalid characters. For example a filename that has a quote in it produces invalid json.

@MartinFillon
Copy link
Contributor Author

Since this is hand rolling the json impl it isn't properly escaping invalid characters. For example a filename that has a quote in it produces invalid json.

Oh Thanks didn't thought of that gonna take a look on how to fix it.

@MartinFillon MartinFillon force-pushed the format-json-option branch 2 times, most recently from f69b195 to 74f5098 Compare May 31, 2024 20:05
MartinFillon and others added 16 commits June 19, 2024 11:30
@MartinFillon
feat(json): starting to parse json as per asked in #768
18cace2
@MartinFillon
refactor: refactored the way string are created to follow up
f99473c
@MartinFillon
docs(json): changed docs to add new arg
e2d4281
@MartinFillon
fix: clippy warnings
154eba6
@MartinFillon
test: regenerated in place tests
12062f6
@MartinFillon
feat(json): updated format to fit requirements
4601ecf
@MartinFillon
feat(json): made json available for every output type
f3e7c8f
@PThorpe92 @MartinFillon
fix(json): correct brace formatting output for json flag
214557f
@MartinFillon
refactor: moved things around to easy up the tree implementation
b337360 
I am still having trouble to implement json tree with the current data
If I can't find anything I might refactor a bit of the code to have
a different type of data structure saved to then be printed at once
@PThorpe92 @MartinFillon
fix: rename variable and remove unneeded fn parameter in details
8def9e3
@MartinFillon
feat: first recursive is done need to be able to have depth
c7f4c62
@MartinFillon
feat: tree is growing
e5c99f7 
Tree works for small trees but still have problems on big complex trees
such as eza dir.
@MartinFillon
refactor: separated json in another file
e9a29fa
@MartinFillon
feat: add recurse option
9ca2894
@MartinFillon
chore: formatted code
de2e533
@MartinFillon
fix: fixed build error
f2476cb
@PThorpe92
Copy link
Member

Have we considered just creating a struct and impl serde::Serialize (nu_ansi_term supports this for the ansi codes) and doing to_string_pretty()?

@MartinFillon
Copy link
Contributor Author

Well, the thing is that the way we are storing everything right now is not really the best, I thought of using this but implementing it gave me more headaches than anything. And I was stuck between refactoring everything just for this to work, or doing this way. As I didn't had the head time to refactor I didn't move with it, but if we take the time to refactor, and stop storing the strings in a vector but a better table that would make it better clearly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cafkafk cafkafk Awaiting requested review from cafkafk cafkafk is a code owner

@PThorpe92 PThorpe92 Awaiting requested review from PThorpe92 PThorpe92 is a code owner

@S1ngularity96 S1ngularity96 Awaiting requested review from S1ngularity96

At least 1 approving review is required to merge this pull request.

Assignees

@MartinFillon MartinFillon

Labels
Arguments Relates to arguments parsing/modifying/Adding enhancement New feature or request features › ui
Projects
Eza Pull-Requests
Status: 🆕 New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add json formatted output option
4 participants
@MartinFillon @PThorpe92 @S1ngularity96 @lespea