Skip to content

added azure_pipeline in this repo #29

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

Merged
merged 18 commits into from
Aug 15, 2023
Merged

Conversation

zaowang-ms
Copy link
Contributor

This is separated from #28

@zaowang-ms
Copy link
Contributor Author

@bheftel Hi Brannon, please review this PR when you have time. Also let me know if you need more context!

Copy link

@bheftel bheftel left a comment

Choose a reason for hiding this comment

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

Hey @zaowang-ms - this looks reasonable to me, but you're right that I don't have a lot of context. @ornj probably has more context, and that is probably more important than familiarity with typescript syntax.

@zaowang-ms
Copy link
Contributor Author

Hey @zaowang-ms - this looks reasonable to me, but you're right that I don't have a lot of context. @ornj probably has more context, and that is probably more important than familiarity with typescript syntax.

Hi Brannon, thanks for reviewing it although I guess it can be a bit too much to review as we are introducing a brand-new project in this repo. As we discussed above, it would be muchly appreciated if @ornj has any contextual comments or thoughts on this PR.

Copy link
Member

@ornj ornj left a comment

Choose a reason for hiding this comment

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

General comments:

  • Please review your code and documentation for things like spelling errors
  • I see a bunch of variation in indentation, line breaks, etc. Suggest adding a formatter to keep things consistent. It might seem like a silly thing but it makes code easy to read and easier to contribute to.
  • Consider adding a linter. I don't know what the cool kids use these days but way back when I was partial to eslint.

@zaowang-ms
Copy link
Contributor Author

Hi @ornj , thanks for the careful reviews. I have made correspond changes and I think feedbacks from your side helped increase the codebase a lot. Could you take a look and see if there are any other changes needed?

Also, I think I may need a separate PR to create a shared Readme.md and also make some modifications on the release json. Once all that are done, we're good to release it.

@zaowang-ms zaowang-ms merged commit 78e27e0 into main Aug 15, 2023
@zaowang-ms zaowang-ms deleted the zaowang/add-azure-pipeline-repo branch September 11, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants