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

save #381

Merged
merged 2 commits into from
May 31, 2020
Merged

save #381

merged 2 commits into from
May 31, 2020

Conversation

Lempireqc
Copy link
Contributor

@Lempireqc Lempireqc commented May 26, 2020

Fix for: #351

@StefH
Copy link
Collaborator

StefH commented May 26, 2020

Hello @Lempireqc and @JonathanMagnan,

1] It may be a good idea to give a meaningful description to this pull-request (and also to #380). This helps for understanding what has been added to the project.

2] Also it can be useful to link a PR to the related issue, in that way tracking the PR and the issue is easier.

3] And maybe add/update unit-tests for this one and the previous? Or will this be done in another PR?

@JonathanMagnan
Copy link
Member

Hello @StefH ,

You are right. I was planned to give more information on release note but it should be in the pull as well. I will update it when I will release this weekend. We maybe try sometime to save too much time ;)

This one already has a unit test for both pull: https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/381/files#diff-b752cf67daca39a6e6a249cc10ad554f

@StefH
Copy link
Collaborator

StefH commented May 27, 2020

For 1]
Previously I did generate the release-notes automatically using GitHubReleaseNotes.exe , https://github.com/zzzprojects/System.Linq.Dynamic.Core/blob/master/GitHubReleaseNotes.txt
These release notes were based on the Tags, PullRequests and Issues.
And the PR's and Issues need to have the correct Label applied.

The file looks like : https://github.com/zzzprojects/System.Linq.Dynamic.Core/blob/master/CHANGELOG.md

For 3]
I see indeed that you have a unit-test, however, I was not expecting to find that file with that name in that location.
It's easier to keep the unit-tests grouped in the same structure, so you can see in one quick overview what tests are there.

@JonathanMagnan JonathanMagnan merged commit 79cb06d into zzzprojects:master May 31, 2020
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.

3 participants