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

Enabling github templates for Issues and PRs #1893

Merged
merged 12 commits into from
Aug 10, 2017
20 changes: 20 additions & 0 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
For bug reports, please respect the following guidelines and check the boxes accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we prefer a badly written bug reports to no bug report at all? If so, maybe say so here ("these are suggestions, not rules" or something).

Copy link
Member

Choose a reason for hiding this comment

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

In my experience badly written bug reports are mostly useless, so I'd prefer to keep this as it is.


For questions of the type: "How can this be done?", please consider posting them on Stackoverflow.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know SO so well, but can we make a link to the relevant "tag" or whatever they call it, or possibly tell people to tag their relevant questions 'mlr'?


For everything else ignore this template.

## Bug report

- [ ] Start a new R session
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly ask people to use --vanilla or something? I don't know how it looks in RStudio, but if people use the command line R, they might have some .RData which they overlook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Stuff in the .RData should not compromise mlrs functionality. Loaded namespaces are another thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Willing to bet?

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

library("mlr")
library("parallelMap")
parallelStartMulticore(2)
parallelMap(function(x) resample("classif.IBk", pid.task, cv5), 1:2, simplify=FALSE)

works if run in R --vanilla, but does not work when run in an R session that has an "RWeka" model in its save file. If you do the above after running

library("mlr")
m = train("classif.IBk", pid.task)
q("yes")

it will hang.

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

I guess most things are obviously bugs or non-bugs when run in any kind of environment; requiring the user to start a new R session (or maybe not even that) will be good enough for like 99% of all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't bet 😉 That is a really strange behavior. Do we have an issue for that?

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

This is mostly described in #1898; afaics there is not much we can do about it. There is also a way of triggering #1379 this way, which is really a PITA if you don't know what's going on.

What I think happens is that objects in .RData that reference namespace environments in some way cause these namespaces to load.

Copy link
Member

Choose a reason for hiding this comment

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

we dont want to overcomplicate this here. people should not be forced to run vanilla.
yes that CAN influence stuff, but the probability as very low. as low as the amount of people who will not be confused by this request.

- [ ] Install the latest version of mlr: `update.packages(oldPkgs="mlr", ask=FALSE)`
- [ ] If you use a GitHub install of mlr: `devtools::install_github(c("BBmisc", "ParamHelpers", "mlr"))
Copy link
Contributor

Choose a reason for hiding this comment

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

closing backtick is missing

Copy link
Contributor

Choose a reason for hiding this comment

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

So how is someone supposed to tick all these boxes? Install the latest cran mlr version and then install it from github again?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make one sentence

- [ ] [Give a minimal reproducible example](http://stackoverflow.com/a/5963610)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not linking to an explanation what exactly comprises the kind of example we want. If I were just on the fence of reporting a bug, someone expecting me to read all of this this would push me towards not doing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, possibly explicitly state to surround code blocks with ```R. This is actually the one thing I think people should do better in the bug reports I've seen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove the link. People will hopefully know what a reproducible example is.

- [ ] run `sessionInfo()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not just get really cluttered bug reports when we ask for this? When was the last bug where this would have helped us recognize the issue faster?

Copy link
Member

Choose a reason for hiding this comment

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

the sessionInfo helps with problem in eg learners. is see @mb706 point, but IMO it is easy to scroll over the sessionInfo. it dont think it hurts


### Minimal reproducible example:

```r
lrn = makeLearner("classif.lda")
resample(learner = lrn, task = iris.task, resampling = cv10)
```
6 changes: 6 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
We are always happy to receive pull requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we decide whether a PR gets merged? Bugfix vs. added features? Relevance to what people within the mlr team are doing? Code quality? Maintainability? Willingness of the submitter to support their part of the code? Unspoken feelings of obligation when someone obviously puts lots of effort into his PR? Hope to get someone invested in mlr to make him post higher quality PRs in the future? (I actually don't know, but maybe we should make some of these explicit here.)

Copy link
Member

Choose a reason for hiding this comment

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

I think the criteria at the moment is that somebody with merge privileges has a look at it...


Please make sure you have read our coding guidelines: https://github.com/mlr-org/mlr/wiki/mlr-Coding-Guidelines
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a short bullet point list of the most important points, in particular the points that are important for people doing only small fixes and unwilling to read through all of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to add some.

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

Lets brainstorm!

  • code style
    • most obvious deviation from widespread R usage elsewhere is using = instead of <-
    • functions use camelcase, variables use dotted case
    • use thirdparty/quicklint to quickly check for coding guidelines
  • tests
    • new functionality should be tested
    • bugfixes should have a test that catch the old bug, if reasonable

I'd assume most people who are at peril of submitting PRs to us (1) know how to use git and (2) (now that we don't rebase any more) know our process of doing PRs, so maybe add an encouraging "if you just do a small fix this is probably all you need to know"

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Use devtools::test(path, filter = pattern) to run a single test file

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a short bullet point list of the most important points, i

no, lets please NOT become redundant. people can click on links

Copy link
Member

Choose a reason for hiding this comment

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

i am all for adding stuff which is not in the other docs


Also it's helpful to find a buddy from the core team who helps you getting your PR merged.
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is this "core team", and how will the contributor in spe know?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe link to this list? However, these are all contributors and not just the 'core' guys (I do not know myself who the core guys are)

You might want to join our slack channel!
Copy link
Contributor

Choose a reason for hiding this comment

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

How? Why?