Skip to content

Conversation

@nwithan8
Copy link
Contributor

@nwithan8 nwithan8 commented Jul 1, 2022

Description

  • New Makefile with common commands
  • Split build.bat into multiple smaller Batch scripts (allow to be run individually or via make)
  • Fix compile tests in F# and VB unit tests

Testing

  • All Unix-compatible scripts work.
  • Will test on Windows machine.

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

- Split build.bat into multiple smaller Batch scripts (allow to be run individually or via make)
- Fix compile tests in F# and VB unit tests
@nwithan8 nwithan8 requested review from Justintime50 and jchen293 July 1, 2022 22:47
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

Windows doesn't have access to make out of the box whereas many unix systems do. You'll want to install make on Windows and ensure these commands are cross compatible, otherwise there isn't much worth in adding these in: https://community.chocolatey.org/packages/make

I'll defer reviewing till Windows support has been confirmed as I'm sure there are a couple tweaks left.

@Justintime50 Justintime50 marked this pull request as draft July 5, 2022 15:45
@nwithan8 nwithan8 marked this pull request as ready for review July 5, 2022 18:45
@nwithan8 nwithan8 requested a review from Justintime50 July 5, 2022 18:45
nwithan8 and others added 3 commits July 5, 2022 17:31
- Makefile step to check Batch scripts
- Checking Batch scripts added as CI step (won't fail)
- Makefile step still exists for manual run
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

All is fine; however, I'd prefer that check_scripts is renamed to something like lint_scripts so it is more easier for searching, understanding, and uses the same language we are accustomed to.

@nwithan8 nwithan8 requested a review from Justintime50 July 6, 2022 17:09
@nwithan8 nwithan8 merged commit 0b606c4 into master Jul 6, 2022
@nwithan8 nwithan8 deleted the makefile branch July 6, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants