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

Integrate shfmt #128

Merged
merged 15 commits into from
Jul 23, 2024
Merged

Integrate shfmt #128

merged 15 commits into from
Jul 23, 2024

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented May 29, 2024

The mechanism can support multiple formatters, and selects the first one available in current environment

Closes #103

@b1ek b1ek added the enhancement New feature or request label May 29, 2024
@b1ek b1ek modified the milestone: Stable release May 30, 2024
@arapower
Copy link
Contributor

I've updated to the latest code with git pull.
After installing shfmt locally and running cargo test, I encountered the following errors:

failures:

---- tests::validity::array_init stdout ----
thread 'tests::validity::array_init' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::validity::bool stdout ----
thread 'tests::validity::bool' panicked at src/tests/validity.rs:53:5:
assertion `left == right` failed
  left: ""
 right: "1"

---- tests::validity::add stdout ----
thread 'tests::validity::add' panicked at src/tests/validity.rs:23:5:
assertion `left == right` failed
  left: ""
 right: "60"


failures:
    tests::validity::add
    tests::validity::array_init
    tests::validity::bool

test result: FAILED. 87 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 12.05s

error: test failed, to rerun pass `--bin amber`

At other times, the following error may occur during execution.

failures:

---- tests::validity::array_assign stdout ----
thread 'tests::validity::array_assign' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- tests::validity::array_init stdout ----
thread 'tests::validity::array_init' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::validity::add_arrays_literal stdout ----
thread 'tests::validity::add_arrays_literal' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- tests::validity::array_assign_out_of_bounds stdout ----
thread 'tests::validity::array_assign_out_of_bounds' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- tests::validity::bool stdout ----
thread 'tests::validity::bool' panicked at src/modules/formatter.rs:57:30:
Couldn't spawn shfmt: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- tests::validity::add stdout ----
thread 'tests::validity::add' panicked at src/tests/validity.rs:23:5:
assertion `left == right` failed
  left: ""
 right: "60"


failures:
    tests::validity::add
    tests::validity::add_arrays_literal
    tests::validity::array_assign
    tests::validity::array_assign_out_of_bounds
    tests::validity::array_init
    tests::validity::bool

test result: FAILED. 84 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 11.92s

error: test failed, to rerun pass `--bin amber`

@Ph0enixKM
Copy link
Member

Ph0enixKM commented May 31, 2024

@arapower what is your default shell? It seems that this code doesn't run on bash? Correct me if I'm wrong

@arapower
Copy link
Contributor

@Ph0enixKM
I am using Ubuntu on WSL2.

$ echo $SHELL
/bin/bash

src/modules/formatter.rs Outdated Show resolved Hide resolved
src/modules/formatter.rs Outdated Show resolved Hide resolved
src/tests/formatter.rs Show resolved Hide resolved
@b1ek b1ek requested a review from Ph0enixKM June 5, 2024 13:33
@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jun 5, 2024

@b1ek btw I was thinking since src/modules is a directory for syntax modules - can we also move the files that are not-related to parsing syntax somewhere else? Like for ex. src/utils.

Beacuse src/modules/formatter.rs is more about formatting the output. I have to add a section about what dir means what in contribution section of README.md

@b1ek
Copy link
Member Author

b1ek commented Jun 5, 2024

src/modulesis a directory for syntax modules, so should moveformatter.rs` into a different directory

sure, that makes sense

I have to add a section about what dir means what in contribution section of README.md

wouldn't it be better to create a separate guide for contributing in CONTRIBUTING.md?

@Ph0enixKM
Copy link
Member

Oh yeah. It would. I'll create an issue for that

@Ph0enixKM
Copy link
Member

I've added a draft issue in our project

@Mte90
Copy link
Member

Mte90 commented Jul 4, 2024

Any updates for this?
I have to do mmanually shfmt everytime.

@Ph0enixKM
Copy link
Member

This issue should also cover the installation process of shfmt in the installer script. It should as user if they want to install it. If user has no bash formatted installed - then there shouldn’t be any warning.

I have to do mmanually shfmt everytime.

Could you make an amber script to compile and run and format in the meantime? I think that @b1ek has a lot of things on her plate right now. I don’t want to hurry anyone here as we all have other duties

@Mte90
Copy link
Member

Mte90 commented Jul 5, 2024

The command is shfmt -w script.sh a script seems to much I think

@Mte90 Mte90 removed the request for review from boushley July 16, 2024 13:52
@Mte90
Copy link
Member

Mte90 commented Jul 16, 2024

The code now compiles and I tested locally and works :-D

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 20, 2024

@Mte90 @b1ek We need to resolve conflicts here. Once done we'll merge this

@Mte90
Copy link
Member

Mte90 commented Jul 22, 2024

@Ph0enixKM fixed and tested locally.

src/compiler.rs Outdated Show resolved Hide resolved
src/modules/formatter.rs Outdated Show resolved Hide resolved
src/modules/formatter.rs Outdated Show resolved Hide resolved
src/modules/formatter.rs Show resolved Hide resolved
src/tests/formatter.rs Show resolved Hide resolved
Mte90 and others added 3 commits July 22, 2024 15:15
Co-authored-by: Hubert Jabłoński <[email protected]>
Co-authored-by: Hubert Jabłoński <[email protected]>
Co-authored-by: Hubert Jabłoński <[email protected]>
@Mte90
Copy link
Member

Mte90 commented Jul 23, 2024

So I think that we can approve this PR, later we can create a ticket on how to evolve this integration.

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Looks good enough

@Mte90 Mte90 dismissed KrosFire’s stale review July 23, 2024 11:06

to discuss in a future version

@Mte90 Mte90 merged commit 03c6be2 into amber-lang:master Jul 23, 2024
1 check passed
This was referenced Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Integrate a bash formatter
5 participants