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

fix typos and some whitespace #4

Merged
merged 1 commit into from Apr 20, 2012
Merged

fix typos and some whitespace #4

merged 1 commit into from Apr 20, 2012

Conversation

helix84
Copy link
Member

@helix84 helix84 commented Apr 20, 2012

  • fixes typos almost exclusively in comments and strings
  • fixes whitespace where combination of tabs and spaces may break indentation
  • doesn't touch typos in config property names to prevent breakage

* fixes typos almost exclusively in comments and strings
* fixes whitespace where combination of tabs and spaces may break indentation
* doesn't touch typos in config property names to prevent breakage
@tdonohue
Copy link
Member

Hi helix84,

Wow. In one commit you've attempted to correct the horrible misspellings of years of bad spellers :)

Still in the process of giving this a review (it's a large commit), but so far so good. Looks like the vast majority (99%) of changes are just to comments, which makes it a bit easier to skim through and approve.

One tip for any future "pull requests" you submit:

We recommend submitting the pull request from a branch that is not your "master". This one was submitted from your "master" branch. That's OK, as long as you don't commit anything else to your "master" branch until we accept this pull request (if you did commit something new, those commits would be auto-added to this pull request). For more info see:
https://wiki.duraspace.org/display/DSPACE/Development+with+Git#DevelopmentwithGit-ContributingChanges%2FPatchestoDSpaceviaGitHub

As I said though, so far this looks good. So, hopefully we can just get these spellings fixed once and for all. Thanks for the contribution!

@tdonohue
Copy link
Member

Finished my skim/review. This looks good. I'm going to create a JIRA ticket for it though. Even though it is just fixing typos, this fix is touching a large number of files. So, it'd be good for us to track this change in JIRA.

But, I'll merge in this pull request shortly, after I link it up with JIRA, etc.

Thanks again, helix84! Much appreciated.

@tdonohue
Copy link
Member

This work is now in JIRA as ticket DS-1158:
https://jira.duraspace.org/browse/DS-1158

Will merge this pull request into main codebase shortly & close the JIRA ticket.

tdonohue added a commit that referenced this pull request Apr 20, 2012
fix typos and some whitespace (also tracked in JIRA as DS-1158)
@tdonohue tdonohue merged commit 14f9066 into DSpace:master Apr 20, 2012
@mdiggory
Copy link
Member

Yo man, not to put a damper on this, but I'm not the biggest fan of mass reformatting commits like this early in a development cycle. Its not the greatest having merge changes that are not actual code changes when your working on something more technical. In this regard, I've always aired on the side of accepting a little incorrectness in format as opposed to forcing significant line changes into everyone else's work. If anything, I would prefer to see reformatting commits happen just prior to the release.

This said, I'm hoping git does do a better job on merging these non-code changes back upstream to forks. I will experiment a little on my branches and see how we fare.

Also, I recommend that we take a position in the future that pull requests should be left open longer than 24 hours prior to being merged. It would give folks room to review the changes which means that if something is not agreeable, the original committer/merger does not have to go through the significant labor of unravelling the commit. (not that I'm going to reject this particular one)

Finally, I'm not sure about "ordering" but if you take a pull request that was created "after" an "earlier" one does this create any issues with merging that earlier pull request? See... #3

@tdonohue
Copy link
Member

Sorry, I had the opposite approach -- get this large thing in first, before we start doing much more development here in GitHub :)

If you really need to "undo" this temporarily, then feel free (and reopen JIRA DS-1158). Sorry, this was just me wanting to get in this larger change now, before development for 3.0 really kicked into full gear.

The ordering of pull requests doesn't matter -- they are always taken/merged against the latest "master".

I'm cool with instituting at least a 24 hour review in future

@mdiggory
Copy link
Member

no sweat, its a good test of gits merge capabilities ;-)

@mdiggory
Copy link
Member

Ok... heres the pain point... we have some cases where windows lineffeds were introduced in this request. Likewise, I;ve shifted my approach and attempted to rebase and merge these changes into my local maven-project-consolidation branch.

It is rather a pain, every file I've consolidated is requiring me to resolve the reformatting changes by hand. At least now I have uncovered the above issue in attempting the merge.

<<<<<<< HEAD:dspace-sword-client/dspace-sword-client-xmlui-api/src/main/resources/aspects/SwordClient/sword.js
/*^M

  • The contents of this file are subject to the license and copyright^M
  • detailed in the LICENSE and NOTICE files at the root of the source^M
  • tree and available online at^M
    *^M
  • http://www.dspace.org/license/^M
    */^M
    importClass(Packages.org.dspace.authorize.AuthorizeManager);^M
    ^M
    importClass(Packages.org.dspace.app.xmlui.utils.FlowscriptUtils);^M
    importClass(Packages.org.dspace.app.xmlui.utils.ContextUtil);^M
    importClass(Packages.org.dspace.sword.client.DSpaceSwordClient);^M
    importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectTargetAction);^M
    importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectCollectionAction);^M
    importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectPackagingAction);^M
    importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.DepositAction);^M
    importClass(Packages.org.dspace.content.Item);^M
    ^M

vs my local fork originally having

/*

  • The contents of this file are subject to the license and copyright
  • detailed in the LICENSE and NOTICE files at the root of the source
  • tree and available online at
    *
  • http://www.dspace.org/license/
    */
    importClass(Packages.org.dspace.authorize.AuthorizeManager);

importClass(Packages.org.dspace.app.xmlui.utils.FlowscriptUtils);
importClass(Packages.org.dspace.app.xmlui.utils.ContextUtil);
importClass(Packages.org.dspace.sword.client.DSpaceSwordClient);
importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectTargetAction);
importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectCollectionAction);
importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.SelectPackagingAction);
importClass(Packages.org.dspace.app.xmlui.aspect.swordclient.DepositAction);
importClass(Packages.org.dspace.content.Item);

/**

  • Simple access method to access the current cocoon object model.
    */
    function getObjectModel()
    {
    return FlowscriptUtils.getObjectModel(cocoon);
    }

I'm not sure if the file in DSpace/DSpace got windows linefeeds added to it or what... I really do not recommend developing DSpace on a windows machine, the pain points are immense and the OS in general is a pile of crap.

Finally, Not to sound like a complainer... git does not remove the human need to visually merge conflicting changes, just as many conflicts do occur as compared to an svn merge, resolution is still in a local workspace and then needs to be commited/pushed into your own fork... its not all its cracked up to be...

@mdiggory
Copy link
Member

Sorry, I fear that might have sounded rude, helix84, thanks for your great work in cleaning things up, I think we are just learning to deal with change in git.

I think I did complete a successful rebase of DSpace/DSpace/master into my maven-project-consolidation branch, but i did need to use git push --force to get it into my local fork in the end, not sure if that was the best approach.

https://github.com/mdiggory/DSpace/tree/maven-project-consolidation

Next exercise, I will attempt to setup a pull request for this to DSpace/DSpace/master.

@tdonohue
Copy link
Member

Sounds like we've hit the first example of "line endings" issues with Git/GitHub.

I'll note that the Fedora Developers have developed their own Best Practices for line endings, to avoid these sort of issues. This may be something we need to look at adopting as well, and ensure that all "pull requests" are built using unix style line endings (even if the initial development took place on a Windows box).

Take a look at the Fedora developers' "line ending" guidelines here:
https://wiki.duraspace.org/display/FCREPO/Git+Guidelines+and+Best+Practices#GitGuidelinesandBestPractices-Lineendings

We should make sure we fix any improper line endings that accidentally got into the codebase (via a followup commit as needed, etc), or alternatively "undo" this commit and then redo it with proper line endings. I'm out-of-office this week, so I likely won't be able to look into this any further until next week.

@mwoodiupui
Copy link
Member

Anyone who has watched my patches will know that I don't mind a moderate amount of respelling/reformatting anytime, but I would probably have done this in a number of smaller pieces. That said, thanks for all the cleanups!

lap82 pushed a commit to lap82/DSpace that referenced this pull request Jul 24, 2013
KevinVdV referenced this pull request in KevinVdV/DSpace Aug 5, 2013
[DS-1606] Bring dspace-solr up-to-date with current development environment
lap82 pushed a commit to lap82/DSpace that referenced this pull request Oct 19, 2013
Restyling lookup submission jsp, fix bug to fill data authors in dto, mo...
artlowel referenced this pull request in atmire/DSpace Jun 13, 2014
fix typos and some whitespace (also tracked in JIRA as DS-1158)
artlowel referenced this pull request in atmire/DSpace Jun 13, 2014
[DS-1606] Bring dspace-solr up-to-date with current development environment
artlowel referenced this pull request in atmire/DSpace Jun 13, 2014
Restyling lookup submission jsp, fix bug to fill data authors in dto, mo...
peterdietz pushed a commit to peterdietz/DSpace that referenced this pull request Sep 24, 2014
pull latest changes from DSpace repo
hardyoyo pushed a commit to hardyoyo/DSpace that referenced this pull request Dec 22, 2014
Adding unit test for the date parser
pnbecker referenced this pull request in tuub/DSpace Oct 20, 2015
These fixes make sense, merging, thanks for the help @pnbecker.
pnbecker pushed a commit that referenced this pull request Aug 25, 2016
tdonohue pushed a commit that referenced this pull request Aug 31, 2016
[DS-1814] remove deprecated ConfigurationManager in favour of Configu…
hardyoyo added a commit to hardyoyo/DSpace that referenced this pull request Nov 7, 2016
[DP-24] removed unecessary class from checkbox markup, see DS-3376
mspalti added a commit to hatfieldlibrary/DSpace-Archived that referenced this pull request Nov 17, 2016
* commit 'b021f4eb1673a046ab6d7da2f6699a4e3af9d0aa':
  Added buildlive.properties
kosarko pushed a commit to kosarko/DSpace that referenced this pull request Aug 1, 2017
gkostin1966 referenced this pull request in mlibrary/DSpace May 23, 2023
cleanup of Embargo, Tombstone and Request Copy.
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.

None yet

4 participants