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

Keep freetext in the beginning of a release. #15

Closed
wants to merge 1 commit into from

Conversation

maltevesper
Copy link
Contributor

fixes #13 partially, a pr for cider will follow

There are a few things which are debatable (i.e. do we really need getter and setter for _headers?, the thought behind htas was that it feels a bit dirty to simply keep a list of elements, and maybe the implementation to keep the headers might change, but I am not sure if it would not be better to remove the getter/setter and readd them if needed (KISS?)).

One could look at Section.copy and consider turning it into a copyWith.

@f3ath
Copy link
Owner

f3ath commented Jun 9, 2023

When adding a feature, I suggest starting with a (failing) test illustrating the use case(s). This way we'll have the API and the expectations set for the code changes, otherwise it's hard to speculate about the particular implementation. The next step (for me personally) would be any straightforward implementation which satisfies the test and introduces no untested lines. This command helps with coverage:

dart run coverage:format_coverage -l -c -i .coverage --report-on=lib --packages=.dart_tool/package_config.json | dart run check_coverage:check_coverage

@f3ath
Copy link
Owner

f3ath commented Jun 11, 2023

Speaking of _headers, I don't see the need for getters and setters, let's keep it simple and have a final public property initialized as an empty list.

@f3ath
Copy link
Owner

f3ath commented Jun 12, 2023

Merged in #16

@f3ath f3ath closed this Jun 12, 2023
@maltevesper
Copy link
Contributor Author

Thanks for #16, I was away for the weekend and was about to do as I was told (i.e. add tests), and to cleanup the linter warnings I caused. Guess I got my feature easier, thank you :)

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.

Allow keeping freetext release notes
2 participants