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

postprocessors #383

Merged
merged 44 commits into from
Nov 9, 2024
Merged

postprocessors #383

merged 44 commits into from
Nov 9, 2024

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Jul 31, 2024

this removes code from #128 and closes #377, #149, #107, #95

the implementation is no different than the one from #128, but it also can be defined as a serializeable schema in case we want to make some sort of a config file for those.

closes #476, #377, #149, #107, #95

@b1ek b1ek added enhancement New feature or request compiler labels Jul 31, 2024
@b1ek b1ek requested review from Mte90 and Ph0enixKM July 31, 2024 00:29
@b1ek b1ek self-assigned this Jul 31, 2024
@Mte90
Copy link
Member

Mte90 commented Jul 31, 2024

Can you add a line in the Readme about the bash dependency checker? There is already one about shfmt.

src/main.rs Outdated Show resolved Hide resolved
src/tests/cli.rs Outdated Show resolved Hide resolved
@b1ek b1ek marked this pull request as ready for review September 7, 2024 06:11
@Mte90 Mte90 requested review from mks-h and KrosFire September 9, 2024 08:21
@Mte90 Mte90 mentioned this pull request Sep 9, 2024
src/compiler/postprocess.rs Outdated Show resolved Hide resolved
@Mte90
Copy link
Member

Mte90 commented Sep 19, 2024

I want to report that clippy is warning about some function unused.

@b1ek
Copy link
Member Author

b1ek commented Oct 31, 2024

@hdwalters do the review thing again

src/compiler/postprocess.rs Outdated Show resolved Hide resolved
src/compiler/postprocess.rs Outdated Show resolved Hide resolved
pub fn get_default() -> Vec<Self> {
let mut postprocessors = Vec::new();

let shfmt = PostProcessor::new("shfmt", "/usr/bin/shfmt");
Copy link
Contributor

@hdwalters hdwalters Oct 31, 2024

Choose a reason for hiding this comment

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

Here you're creating both processors, and then immediately removing them according to the CLI options; this is inelegant. Surely it would be better to combine get_default() and filter_default(), and only create the processors you're actually going to need:

pub fn get_processors(filters: Vec<WildMatchPattern<'*', '?'>>) -> Vec<Self> {
    let mut processors = Vec::new();
    if Self::filter_processor(&filters, "shfmt") {
        // Add "shfmt"
    }
    if Self::filter_processor(&filters, "bshchk") {
        // Add "bshchk"
    }
    processors
}

I suspect you did it this way so you could call get_default() in the integration tests, but you could always call this new function with an empty vector:

let default = PostProcessor::get_processors(Vec::new());

Copy link
Member Author

@b1ek b1ek Oct 31, 2024

Choose a reason for hiding this comment

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

this is inelegant; Surely it would be better to combine get_default() and filter_default(), and only create the processors you're actually going to need:

why? its still the same two functions but now it breaks DRY and a little bit more dirty. there are no issues with the current setup

src/compiler/postprocess.rs Outdated Show resolved Hide resolved
src/tests/cli.rs Outdated Show resolved Hide resolved
@b1ek
Copy link
Member Author

b1ek commented Nov 2, 2024

@Ph0enixKM do review!!!!! we're waiting for u

@hdwalters
Copy link
Contributor

@Ph0enixKM do review!!!!! we're waiting for u

He did say "I'll be unavailable for checking PRs starting from today [30/10] until 3/11 too"

@hdwalters
Copy link
Contributor

I'll approve once @hdwalters confirms that these changes work on Windows GitBash too

For some reason, GitHub is not giving me the option to clone this branch using Git; it only suggests gh pr checkout 383. And unfortunately my employer has us so locked down that I can't install the GitHub CLI tool (this is one of the downsides of working for a fintech company).

However, since @b1ek has now rewritten this code, and removed the cfg!(unix) test, I suggest that we just merge it, and fix any problems caused on Windows if they crop up.

.github/workflows/rust.yml Show resolved Hide resolved
src/compiler/postprocessor.rs Outdated Show resolved Hide resolved
src/compiler/postprocessor.rs Outdated Show resolved Hide resolved
@Ph0enixKM Ph0enixKM merged commit 9d81052 into amber-lang:master Nov 9, 2024
1 check passed
@b1ek b1ek deleted the postprocess branch November 9, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Postprocessors
4 participants