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(build): #1094 update formatPython #1106

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

rohaquinlop
Copy link
Contributor

@rohaquinlop rohaquinlop commented Jun 8, 2023

Fix #1094

  • Update formatPython package to use python 3.11
  • Separate the arg from evaluator
  • Allow having several configurations
  • Allow passing isort and black settings to each configuration
  • Add job to dev pipeline
  • Update documentation

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 8, 2023

Hi @rohaquinlop,

First of all, thank you for this contribution!

Now that we're dealing with formatPython, maybe we should:

  1. Separate the arg from the evaluator.
  2. Allow having several configurations.
  3. Allow passing isort and black settings to each configuration.

Ideally, it would look like this:

# makes.nix
{
  formatPython = {
    strict = {
      enabled = true;
      config = {
        black = ./config/black/strict.toml;
        isort = ./config/isort/strict.toml;
      };
    };
    flexible = {
      enabled = true;
      config = {
        black = ./config/black/flexible.toml;
      };
    };
  };
}

You can use other builtins like testPullRequest arg and evaluator as a reference for achieving all of this.

@rohaquinlop
Copy link
Contributor Author

@dsalaza4 Got it! I'm gonna work on it.

One question, is there any documentation to build from source?

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 9, 2023

@dsalaza4 Got it! I'm gonna work on it.

One question, is there any documentation to build from source?

Just install and run m . <job> from your local repo, that should do the trick.

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 9, 2023

@rohaquinlop Let's get rid of the enabled argument, it is unnecessary.

@rohaquinlop
Copy link
Contributor Author

@rohaquinlop Let's get rid of the enabled argument, it is unnecessary.

Ok

@rohaquinlop
Copy link
Contributor Author

@dsalaza4 I have one question, there will be only these two configurations? strict and flexible? Because I've been trying to implement it allowing different config names, but it wasn't successfully.

Here's my current implementation allowing different configuration names:

{
  __toModuleOutputs__,
  formatPython,
  ...
}: {
  config,
  lib,
  ...
}: let
  makeOutput = name: cfg: {
    settingsBlack,
    settingsIsort,
    targets,
  }: {
    name = "/formatPython/${name}";
    value = formatPython {
      inherit settingsBlack;
      inherit settingsIsort;
      inherit targets;
      inherit name;
    };
  };
in {
  options = {
    formatPython = {
      configurations = lib.mkOption {
        type = lib.types.attrsOf (lib.types.submodule (_: {
          options = {
            settingsBlack = lib.mkOption {
              default = ./settings-black.toml;
              type = lib.types.path;
            };
            settingsIsort = lib.mkOption {
              default = ./settings-isort.toml;
              type = lib.types.path;
            };
            targets = lib.mkOption {
              default = ["/"];
              type = lib.types.listOf lib.types.str;
            };
          };
        }));
      };
    };
  };
  config = {
    outputs = __toModuleOutputs__ (lib.mapAttrs' makeOutput config.formatPython.configurations);
  };
}

And I'm getting the following error:

error: The option `formatPython.strict' does not exist. Definition values:
       - In `<unknown-file>':
           {
             config = {
               targets = [
                 "/"
               ];
           ...
# makes.nix
{
  formatPython = {
    strict = {
      config = {
        targets = ["/"];
      };
    };
  };
}

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 10, 2023

@rohaquinlop strict and flexible are not static configurations but rather example names for configurations that receive a given set of formatter configs (black and isort for now). You can create as many configurations as you want within the formatPython block.

# makes.nix
{
  formatPython = {
    my-config-1 = {
      config = {
        black = ./path-in-my-repo-to-a-config.toml;
        isort = ./path-in-my-repo-to-another-config.toml;
      };
    };
    my-config-2 = {
      # No configs provided, use default ones
    };
  };
}

@dsalaza4
Copy link
Contributor

Regarding the error you're getting, I think the reason is that you have a confiugurations block within formatPython that is receiving the configs. Try using formatPython.configurations.strict. In any case, I think we should remove the configurations block and stick to the formatPython.strict format.

@rohaquinlop
Copy link
Contributor Author

@dsalaza4 I'm getting an error and I can't figure out why is it.

Error:

error: getting status of '/nix/store/yd084mqsjqlj6n5wfwmv10x3gnq84p78-makes-ac32mpkr/src/args/format-python/default.nix': No such file or directory

But I already added formatPython to the agnostic.nix file in the path /src/args/ do you have any idea why I'm getting this error?

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 10, 2023

Maybe you're not adding the new files to the repository. Try at least adding all new files to staging with a git add, commiting them would also work.

makes.nix Outdated Show resolved Hide resolved
makes.nix Outdated Show resolved Hide resolved
makes.nix Outdated Show resolved Hide resolved
makes.nix Show resolved Hide resolved
.github/workflows/dev.yml Outdated Show resolved Hide resolved
.github/workflows/dev.yml Outdated Show resolved Hide resolved
.github/workflows/dev.yml Outdated Show resolved Hide resolved
.github/workflows/dev.yml Outdated Show resolved Hide resolved
@dsalaza4
Copy link
Contributor

Please update documentation accordingly: https://makes.fluidattacks.com/api/builtins/format/?h=formatp#formatpython

You can use m . /docs/dev for a local rendering.

@rohaquinlop rohaquinlop force-pushed the rohaquinlop branch 3 times, most recently from 915fd02 to d4ccc22 Compare June 12, 2023 01:39
@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 13, 2023

@rohaquinlop It looks like the DCO of your commit is currently failing: https://github.com/fluidattacks/makes/pull/1106/checks?check_run_id=14193870603

Commit sha: 80e7737, Author: Robin Hafid, Committer: Robin Hafid; Expected "Robin Hafid [email protected]", but got "Robin Hafid Quintero Lopez [email protected]".

@dsalaza4
Copy link
Contributor

dsalaza4 commented Jun 13, 2023

@rohaquinlop
Copy link
Contributor Author

@rohaquinlop
Copy link
Contributor Author

@dsalaza4 Done, already updated mailMap in my other PR too.

@dsalaza4
Copy link
Contributor

Please add yourself to the mailmap: https://github.com/fluidattacks/makes/actions/runs/5245872757/jobs/9503238429?pr=1106

Ok, I'm gonna add me

Still failing? You can test with m . /lintGitMailMap

- Update formatPython package to use python 3.11
- Separate the arg from evaluator.
- Allow having several configurations.
- Allow passing isort and black settings to each configuration.

Signed-off-by: Robin Quintero <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rohaquinlop
Copy link
Contributor Author

Please add yourself to the mailmap: https://github.com/fluidattacks/makes/actions/runs/5245872757/jobs/9503238429?pr=1106

Ok, I'm gonna add me

Still failing? You can test with m . /lintGitMailMap

Thank you, didn't update the author's commit.

Copy link
Contributor

@dsalaza4 dsalaza4 left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you so much for all your work!

@dsalaza4 dsalaza4 merged commit c18b4ab into fluidattacks:main Jun 13, 2023
@rohaquinlop
Copy link
Contributor Author

rohaquinlop commented Jun 13, 2023

LGTM!

Thank you so much for all your work!

Thank you to, your quick responses and your patience with me in this PR and with #1107

@rohaquinlop rohaquinlop deleted the rohaquinlop branch June 16, 2023 19:43
@dsalaza4 dsalaza4 mentioned this pull request Jul 19, 2023
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.

FormatPython fails because does not support new Python syntax
2 participants