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

V1.0 development (just a draft PR to view the diff) #787

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

V1.0 development (just a draft PR to view the diff) #787

wants to merge 27 commits into from

Conversation

PThorpe92
Copy link
Member

this is so everyone can fork this branch, and make their own edits, add whatever features, etc. so instead of a massive PR that is impossible to review, we can each add things incrementally

@PThorpe92 PThorpe92 marked this pull request as draft January 17, 2024 23:22
@PThorpe92
Copy link
Member Author

I see the git history is diverged enough where it might have to be rebased off main to one commit :/

@MartinFillon
Copy link
Contributor

Gonna merge and update it

MartinFillon
MartinFillon previously approved these changes Jan 21, 2024
View reviewed changes
@MartinFillon
Copy link
Contributor

Okay so how are we doing on that v1.0 release ?

@daviessm
Copy link
Contributor

daviessm commented Feb 3, 2024

I really think that clap should be merged prior to v1, there are quite a few things queued up waiting for it and we should aim to get all those merged and tested before declaring we're stable.

MartinFillon
MartinFillon previously approved these changes Feb 3, 2024
View reviewed changes
@PThorpe92
Copy link
Member Author

PThorpe92 commented Feb 3, 2024 •
edited
Loading

I'm all for it. Yeah this is really more about clap and anything that would come along with it that would have breaking changes than it would be actually 1.0

more specifically I just wanted a branch people can fork and push their changes to, instead of having 1 PR and requesting a bunch of changes for one person. I think the sooner we get it merged into main the better. you're absolutely right, from a stability perspective this needs to be in well before 1.0 I was caught up on the breaking change perspective

@MartinFillon
Copy link
Contributor

Well guess this can be made ready for review ?

@PThorpe92 PThorpe92 marked this pull request as ready for review February 14, 2024 14:42
@MartinFillon
Copy link
Contributor

Hey @tertsdiepraam you created a breaking change on time-style argument, could you have a look at it
For an exemple please see the test file named long_time_style_custom_non_recent_and_recent_nix.toml and tell me if its intended.

@tertsdiepraam
Copy link
Contributor

tertsdiepraam commented Feb 14, 2024 •
edited
Loading

@MartinFillon I don't think it's due to my changes. With git bisect I've narrowed it down to this commit where the time-style does not accept "+NON_RECENT\n RECENT" anymore: 47b9e45

@MartinFillon
Copy link
Contributor

Well my bad sorry 😞 I should have bisect more in depth

Gonna fix it

@MartinFillon
Copy link
Contributor

This is ready @cafkafk

@cafkafk
Copy link
Member

cafkafk commented Feb 17, 2024

This is ready @cafkafk

Okay let's get going on review then!

cafkafk
cafkafk requested changes Feb 17, 2024
View reviewed changes
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

First order of business is to resolve clippy lints so CI pass it seems

@MartinFillon
Copy link
Contributor

I can't understand nix flake check outputs. It seems to always fail

@tertsdiepraam
Copy link
Contributor

tertsdiepraam commented Feb 17, 2024 •
edited
Loading

Seems to be a formatting issue. I think nix fmt can fix it, but cargo fmt doesn't seem to fix it, which seems strange, because nix fmt should essentially be running cargo fmt under the hood. Maybe the rust-toolchain.toml file might be ignored by nix and it will therefore run a newer version of rustfmt?

Edit: yeah that seems to be it. I temporarily deleted rust-toolchain.toml and then cargo fmt gives the correct diff.

@MartinFillon
Copy link
Contributor

well thats strange Should I fix it ?

MartinFillon and others added 27 commits September 24, 2024 16:52
@MartinFillon
feat(parser)!: Introduction of clap based parser
a600910 
This pr intoduce clap based parser.
Please note that this is still a wip.

Breaking Changes:
-v no longer lists version.
Version is no longer as exhaustive as before.
Signed-off-by: Christina Sørensen <christina@cafkafk.com>
@orest58008 @MartinFillon
feat: Add Fortran icons
2569110
@orest58008 @MartinFillon
fix: Format the code
ac97e48
@MartinFillon
test: updated tests to fit new features
17c72af
@PThorpe92 @MartinFillon
refactor: add enum arguments for clap parser
9346e7f
@PThorpe92 @MartinFillon
fix: correct test behavior for arg parsing
12dc4a9
@PThorpe92 @MartinFillon
fix: merge conflict issues
785eebc
@MartinFillon
feat(parser)!: Introduction of clap based parser
428c991 
This pr intoduce clap based parser.
Please note that this is still a wip.

Breaking Changes:
-v no longer lists version.
Version is no longer as exhaustive as before.
Signed-off-by: Christina Sørensen <christina@cafkafk.com>
@tertsdiepraam @MartinFillon
refactor: change ArgAction count to normal flags
d1a704a
@tertsdiepraam @MartinFillon
refactor: change type of help field of Options to unit
aeb605b
@tertsdiepraam @MartinFillon
refactor: derive Default on Opts instead custom impl
c5013aa
@tertsdiepraam @MartinFillon
refactor: remove explicit long arguments when they can be inferred
9287d5c
@tertsdiepraam @MartinFillon
refactor: remove unnecessary types and implementations from parser.rs
9c25728
@tertsdiepraam @MartinFillon
refactor: remove Display for ShowWhen and ColorScaleArgs
d3c6d39
@tertsdiepraam @MartinFillon
refactor: cleanup custom ValueEnum implementation
c5c7d6f
@tertsdiepraam @MartinFillon
fix: allow arguments to be passed multiple times
a9df7a8
@MartinFillon
refactor: modified args to feat actual args and runned formatter
0c0cf5e
@MartinFillon
refactor: linting of the code with clippy
a4a3d1e
@MartinFillon
fix: fixed custom time-style
65dc8d1 
implemented a typed value parser to have everything working
@MartinFillon
test: updated tests
70883df
@MartinFillon
refactor: fixed clippy lints
7d6cd61
@MartinFillon
feat: added absolute flag to he parser
f119992
@MartinFillon
fix: clippy linting
03eee54
@MartinFillon
test: redumped powertest
490cccd
@MartinFillon
fix: add clap parser use to be able to compile
ee441e2
@MartinFillon
chore: update tests
9e6849d
@MartinFillon
fix(clippy): clippy lints were broken
54bda48
@MartinFillon MartinFillon self-assigned this Sep 24, 2024
@MartinFillon MartinFillon added this to the v1.0 milestone Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinFillon MartinFillon MartinFillon left review comments

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

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

Requested changes must be addressed to merge this pull request.

Assignees

@MartinFillon MartinFillon

Labels
None yet
Projects
Eza Pull-Requests
Status: 🔖 Ready
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@PThorpe92 @MartinFillon @daviessm @tertsdiepraam @cafkafk @orest58008