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

Remove comment left in code #32

Merged
merged 6 commits into from
Oct 21, 2021

Conversation

scribe
Copy link
Contributor

@scribe scribe commented Oct 17, 2021

This is pretty trivial, but I didn't realize structopt would include the possible values until I played with it more later.

This PR removes two comments that are no longer accurate after #31. My bad for leaving comments like that.

@scribe scribe requested a review from Tamschi as a code owner October 17, 2021 03:57
Copy link
Owner

@Tamschi Tamschi left a comment

Choose a reason for hiding this comment

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

I should probably have been more thorough in my review yesterday too, but I got a bit too excited about actually getting a pull request.

#31 (review) already shows the list of possible values in the output, so I don't have to run the program to review this PR (so you didn't cause noticeable extra work for me).

@Tamschi
Copy link
Owner

Tamschi commented Oct 17, 2021

I'll wait a day before merging this PR in case anything else comes up, since that seems like a good idea in general.

I'm not sure if you've read CONTRIBUTING.md, but you're invited to add yourself as copyright holder to both license files if you'd like.

You could also add a CHANGELOG.md entry (for both of your PRs combined). I use the following format for upcoming versions:

## next

TODO: Date

- Revisions:
  - change title (contributed by @<your GitHub @> in #<PR #> and #<PR #>)
    > Further description if necessary.

But this is optional. I'll otherwise add it shortly after merging this pull request here.

@Tamschi Tamschi added state: approved Approved to proceed. type: upkeep Converting measurements, reorganizing folder structure, and other necessary tasks. labels Oct 17, 2021
@Tamschi
Copy link
Owner

Tamschi commented Oct 17, 2021

(I won't add the hacktoberfest-accepted label to avoid accidentally encouraging others to split their pull-requests, but it will likely still count towards Hacktoberfest due to the the repository topic. Which is fair to me - thanks for coming back after my hasty merge yesterday and for teaching me about a structopt feature I didn't know about.)

@Tamschi Tamschi self-requested a review October 17, 2021 10:44
Copy link
Owner

@Tamschi Tamschi left a comment

Choose a reason for hiding this comment

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

I just updated the repository to my newest project template, so if you update your PR with those changes, the failing check should disappear. (This is optional.)


I also just realised I have the --help output in README.md under Usage. (Sorry, it's been a while since I really worked on this project. I should have requested this change in #31.)

Please update that code block so that it reflects your previous PR.

@Tamschi Tamschi added the type: documentation Related to documentation and information. label Oct 17, 2021
@scribe
Copy link
Contributor Author

scribe commented Oct 17, 2021

I'd be happy to do all of the above to help out!

@Tamschi
Copy link
Owner

Tamschi commented Oct 17, 2021

Awesome, thank you so much, but also remember to take it easy 😅

(I recently overworked myself a bit by having too many side projects at once and it wasn't a pleasant experience.

I don't know how much unpaid open-source work you've done so far, but there aren't any deadlines or quotas here.
(I'm assuming you're somewhat new at this though, since Hacktober is often about being a safe space for new contributors.)

I think the only expectation, if there is any, is to avoid causing harm with your code for personal gain and such,
and maybe some base level of contribution quality (that you're very easily clearing here, though the detailed requirements differ between projects, so it's important to dig a bit to see if there's some specific workflow. For my own repositories, I generally only have a few guidelines and some basic conduct rules though).)

@scribe
Copy link
Contributor Author

scribe commented Oct 18, 2021

This might be a silly question, but do you have a preferred format for adding to the copyright? Should it be on a line by itself, or somewhere else?

@Tamschi
Copy link
Owner

Tamschi commented Oct 18, 2021

This might be a silly question, but do you have a preferred format for adding to the copyright? Should it be on a line by itself, or somewhere else?

I didn't think to explain this since it's been a while, but I remember spending a few hours doing research when I published my first OSS project. Explaining licensing formalities is probably a bit too much to add to CONTRIBUTING.md, but I'll see if I can add a link to an explanation in this regard to my project template.

For now, and this isn't quite general advice, since other projects handle this differently, but for mine:


You've likely found them, but the copyright holders lists are currently the following lines:

Copyright 2020-2021 Tamme Schichler <tamme@schichler.dev>

Copyright (c) 2020-2021 Tamme Schichler <tamme@schichler.dev>

with https://github.com/Tamschi/reserde/blob/a1a502d83bb4447fd585ae7108ce86d03fe10636/COPYRIGHT.md tying those licenses together. You should read these files if you aren't familiar with them already.

My projects currently all list copyright holders individually, so please add yourself in a new line directly below each of these.

I personally just went with the suggested format, but this is flexible: You can for example not add your email address and/or (at least by my local laws) use a nickname you're recognised under. Functionally, stating your copyright like that makes it easier for you to enforce it if anyone breaks the package license, and gives me (stronger) assurance that there aren't any licensing issues with your contributions.
I've plastered the contribution agreement all over the repository though, so hopefully it wouldn't be an issue for me even otherwise 😅 (I'm not a lawyer etc.)

Formatting-wise, go with what looks tidy to you without modifying other lines.
I'll suggest a change through the review function in case anything is amiss in that regard.

It's possible that someone down the line will remove one of the licenses, since this crate is dual-licensed under either of "MIT OR Apache-2.0", but they have to retain at least one of these copyright holders lists, or get everyone to agree individually to change it. You can always change your own entries, of course(, at least as far as I'm aware).


There's also the authors field in Cargo.toml, but I see this one more as an invitation to contact me about issues with the crate, so you may not want to add yourself there unless you really want to provide technical support to users.

The field is also technically deprecated, so it's possible I'll remove it in the future and have my contact info only elsewhere in the documentation. (See here for which files I include in the package on crates.io.)


Generally speaking, if you see contributors lists with individual names, it should be safe to add yourself when contributing. It's usually good form for new contributors to add themselves last, so that the original author(s) are listed first.

Also, and I personally chose not to do this since I think it clutters the code a bit, but if a project uses per-file licensing headers, then add yourself only to the files you modified. You'll see this in Linux a lot for example, but it seems to be uncommon for Rust projects.

Adding yourself as copyright holder for typo-fixes or formatting PRs would probably look a bit odd, so I avoid doing that.
(These license lists are a little bit like the OSS version of film credits, there's a tiny bit of clout to being a contributor on a successful project.)

Smaller projects often don't have explicit license formatting guidelines, but since getting contributions is often a rare and precious event for them, it's fine to guess a bit. The maintainer(s) will most likely point it out if there's any issue.

@scribe
Copy link
Contributor Author

scribe commented Oct 18, 2021

I really appreciate you taking some time to explain that to me. I'll be better equipped to help in the future because of you.

@Tamschi Tamschi self-requested a review October 19, 2021 06:53
Copy link
Owner

@Tamschi Tamschi left a comment

Choose a reason for hiding this comment

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

(I'm doing this on mobile, so I hope it comes out right.)

Thanks, this takes care of everything except the changelog entry, which is often added by maintainers anyway. I've left a comment where I need clarification, but overall there aren't any blockers now.

LICENSE-APACHE Outdated Show resolved Hide resolved
@Tamschi
Copy link
Owner

Tamschi commented Oct 19, 2021

I really appreciate you taking some time to explain that to me. I'll be better equipped to help in the future because of you.

I'm just paying it forward, I think. Others have helped me with my PRs in the past and I'd appreciate if you'd do the same in the future, in case you decide to publish your own software this way and happen to get a contributor new to OSS.

@scribe
Copy link
Contributor Author

scribe commented Oct 20, 2021

I’d be happy to add the change log. I don’t want to leave extra work

Copy link
Owner

@Tamschi Tamschi left a comment

Choose a reason for hiding this comment

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

I've added a few suggestions regarding the changelog entry, to make it more consistent with how I usually write them.

I'll likely have some time to make a release on Thursday afternoon (CEST, so about 30 hours from now). I'll merge the PR at that point and maybe add a little of final polish (in general, I usually quickly go over the code when I publish after not touching the project in a while, in case I spot something I can improve now).

Please let me know if you'd like to avoid being credited in the changelog. The entries will also appear in the Git tag and from there (eventually, when I get around to automate that) in generated GitHub Release notes for this release.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Tamschi
Copy link
Owner

Tamschi commented Oct 20, 2021

I’d be happy to add the change log. I don’t want to leave extra work

Oh, and don't sweat it! It's pretty much impossible to turn in code in the exact way the maintainer would have done it.

Sometimes they'll leave it as-is, sometimes they quietly adjust it a bit (I usually just do that with things like the changelog, but in this case I decided to explain my changes a little more) and sometimes they'll suggest changes like in this case (and rejecting those is often okay! Just add a comment regarding why you wouldn't do something the suggested way and the maintainer should often resolve that discussion once it's clarified).

(I used a batch-commit to apply my review suggestions from Tamschi#32 (review) here.)
@Tamschi Tamschi merged commit 00f2c70 into Tamschi:develop Oct 21, 2021
@Tamschi Tamschi added this to the v0.0.3 milestone Oct 21, 2021
@Tamschi
Copy link
Owner

Tamschi commented Oct 21, 2021

I've published v0.0.3 (and made it a proper GitHub "Release" to celebrate my first received contribution in quite a while) 🎉

Pushing the release tag also created a project chat due to my new Zulip automation.
Feel free to drop by if you want, especially if you use reserde as end user and run into any difficulties with it.

@Tamschi
Copy link
Owner

Tamschi commented Oct 21, 2021

Edited the release into v0.0.4 because I'd messed up a bit when I relicensed the program previously and only noticed just now 😅

You can find it here: https://github.com/Tamschi/reserde/releases/tag/v0.0.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: approved Approved to proceed. type: documentation Related to documentation and information. type: upkeep Converting measurements, reorganizing folder structure, and other necessary tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants