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

fix: tryParse line-length to int when it's not already an integer #708

Conversation

jkoenig134
Copy link
Contributor

Description

The flag --line-length was added, but it seems like it doesn't work as ArgParser doesn't seem to transform this param to int by default. This fix takes the String argument and tries parsing it to an int.

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, it would be good with a test for this though

@jkoenig134
Copy link
Contributor Author

@spydon I think the main issues why this occurred is that #689 didn't add a test. For protecting this command for regression issues I'm totally with you, I am open looking into the testing and adding tests for this.

@utamori
Copy link
Contributor

utamori commented Apr 26, 2024

@jkoenig134 thank you!
I tried to add a test, but it didn't work.

I just made the following modification and was able to confirm that it works correctly at hand.

    final lineLength =
        switch (int.tryParse(argResults?['line-length'] as String)) {
      final int length => length,
      _ => null,
    };
melos activate
melos foramt --line-length 120 --set-exit-if-changed

@jkoenig134
Copy link
Contributor Author

@spydon added the test

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Super, thanks!

@spydon spydon enabled auto-merge (squash) April 26, 2024 08:44
@jkoenig134
Copy link
Contributor Author

@spydon as a follow-up i would really like to add setExitIfChanged and lineLength to the config.

I am mostly though with that. As it is not only a Bugfix - can I PR this?

@spydon spydon merged commit 35ef462 into invertase:main Apr 26, 2024
10 checks passed
@spydon
Copy link
Collaborator

spydon commented Apr 26, 2024

@spydon as a follow-up i would really like to add setExitIfChanged and lineLength to the config.

I am mostly though with that. As it is not only a Bugfix - can I PR this?

Sure, sounds good!

@jkoenig134 jkoenig134 deleted the fix/make-parse-line-lenght-when-its-not-an-integer branch April 26, 2024 11:02
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.

4 participants