Old changes I made for case insensitivity - Githubissues

peterblattmann / SWATH2stats

https://peterblattmann.github.io/SWATH2stats/

1 stars 1 forks source link

Old changes I made for case insensitivity #2

Closed abelew closed 4 years ago

abelew commented 5 years ago

I had forgotten about this fork. It is highly likely that you will not want to merge these changes, but they might be interesting to look at. When I was trying to learn how to preprocess some SWATH data, I spent some time working with this code and made the following changes to make it easier for me to step through it:

Chose a single coding style, it mostly agrees with what was already there.
Converted the man/ files to roxygen2 docstrings and ran roxygen2::document(). Thus hopefully I did not change any of the outputs significantly except a couple of punctuation changes.
(The only substantive change) Made the import function(s) and downstream functions case insensitive. I had some problems because the capitalization of columns in my data and metadata were different.

peterblattmann commented 5 years ago

Thank you very much and I will have a look at it in the coming days.

peterblattmann commented 5 years ago

Hi Ashton, I started to look at your pull request. First of all, you did a fantastic job and it is huge. I was wondering how to go best about it. Shall I give comments on the request with the goal to finally merge it or shall I implement the changes I like in an own commit? Open for suggestions how to best deal with this as I'm not sure what is the best way forward.

peterblattmann commented 5 years ago

If we do the former, I think we need to split the pull request into smaller more manageable ones? 1.) Change to roxygen2 2.) making sample annotation function more general 3.) ...

What is the Makefile for? Why do we need this in the package?

peterblattmann commented 4 years ago

I worked now on the sample_annotation file and made some adjustments. Let me know if this is practical like this or if I should directly change the things and commit myself?

abelew commented 4 years ago

Greetings! Whatever is best for you. I am somewhat away for a few days, so it may be best for you to perform the commits. As an arbitrary aside, in other R code I have changed most of my class() checks to be some variant of 'if ("someclass" %in% class(data))' so that it is simpler to read in cases like data.tables which are derived from dataframes. Once I am back for real, if there are any tasks which would be most helpful/useful for you, I would be glad to help.

peterblattmann commented 4 years ago

Can you give me access to your pull request as described in this post? Then I can do the adaptions directly in your pull request. Thanks! https://github.blog/2016-09-07-improving-collaboration-with-forks/

abelew commented 4 years ago

I think I am being particularly dense, but it looks like that when I pulled up the pull request, the 'allow edits from maintainers' option was already on. Is there something else I missed?

peterblattmann commented 4 years ago

Ok, great if this was done already. I was not sure and thanks for the quick reply.

peterblattmann commented 4 years ago

Another question: If I pull now your tree (abelew/SWATH2stats) I do not see the changes of the first larger pull request. Also if I look at the tree, I only see the commits from your pull request (see screenshot) and for example the sample_annotation.R has not the option "condition_column" in the function. So I am a little bit puzzled if these commits (e.g. 41b10d1) are not contained in this tree or if you changed it afterwards back again?

peterblattmann commented 4 years ago

Thanks again @abelew for the contribution! The move to roxygen has been commit in v1.19.1 (https://github.com/peterblattmann/SWATH2stats/commit/56f56c8a0e5e6fce52e8fafaeb2ce3eeff97a9c2)