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: support for melos command within script steps #683

Merged

Conversation

jessicatarra
Copy link
Contributor

Description

This pull request introduces support for melos command within script steps, as suggested in issue #653. An example configuration demonstrating this new capability is as follows:

scripts:
  pre-commit:
    description: pre-commit git hook script
    steps:
      - echo 'hello world'
      - format --output none --set-exit-if-changed
      - analyze --fatal-infos

I'll need some help on determining the appropriate range of commands to support. Should we use every command possible?
Currently, the following commands are under consideration:

    const melosCommands = {
      'analyze',
      'format',
      'bs',
      'bootstrap',
      'clean',
      'list',
      'publish',
    };

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

@jessicatarra jessicatarra changed the title Feat: steps script support for commands feature: support for melos command within script steps Mar 29, 2024
@jessicatarra jessicatarra changed the title feature: support for melos command within script steps feat: support for melos command within script steps Mar 29, 2024
In order to prioritize script names that are identical to melos commands. Additionally, add a test for this scenario.
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!

@spydon spydon enabled auto-merge (squash) March 29, 2024 16:52
auto-merge was automatically disabled March 29, 2024 17:00

Head branch was pushed to by a user without write access


Analyzing ....

info - ${currentPlatform.isWindows ? 'packages\a\main.dart' : 'packages/a/main.dart'}:3:13 - Don't invoke 'print' in production code. Try using a logging framework. - avoid_print
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aah the pain of Windows, this has to have two backslashes \\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh I see, hopefully the last commit solves this issue!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason, even though I added the two backslashes, the error seems to be something else @spydon:

              'warning - pubspec.yaml:6:11 - The path \'d:\\a\\melos\\melos\\packages\\melos\' isn\'t a POSIX-style path. Try converting the value to a POSIX-style path. - path_not_posix\n'
              '   info - packages\\a\\main.dart:3:13 - Don\'t invoke \'print\' in production code. Try using a logging framework. - avoid_print\n'
              '   info - packages\\a\\main.dart:5:10 - Missing a newline at the end of the file. Try adding a newline at the end of the file. - eol_at_end_of_file\n'
              '\n'
              '3 issues found.\n'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spydon I updated the test and use a list script instead of the analyze one to avoid all these issues related to windows environments, and still testing the right scenario, which is prioritazing custom scripts instead of Melos commands when scripts with same names as the commands are used. commit e1d2e96

@jessicatarra jessicatarra force-pushed the feat/steps-script-support-for-commands branch 3 times, most recently from f2acd3a to 2e79c0d Compare March 29, 2024 17:19
@spydon spydon enabled auto-merge (squash) March 29, 2024 17:22
auto-merge was automatically disabled March 29, 2024 17:41

Head branch was pushed to by a user without write access

@jessicatarra jessicatarra force-pushed the feat/steps-script-support-for-commands branch 2 times, most recently from 8166908 to e5c7655 Compare March 29, 2024 17:45
@jessicatarra jessicatarra force-pushed the feat/steps-script-support-for-commands branch 3 times, most recently from 6f1c9eb to efb4a11 Compare March 29, 2024 18:38
@jessicatarra jessicatarra force-pushed the feat/steps-script-support-for-commands branch from efb4a11 to e1d2e96 Compare March 29, 2024 18:50
@spydon spydon merged commit a1da197 into invertase:main Mar 29, 2024
10 checks passed
@xsahil03x xsahil03x mentioned this pull request Apr 1, 2024
1 task
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.

None yet

2 participants