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

[WIP] Django style unchained. #3655

Merged
merged 1 commit into from
Feb 6, 2018
Merged

[WIP] Django style unchained. #3655

merged 1 commit into from
Feb 6, 2018

Conversation

tobiasdiez
Copy link
Member

Code examples are better than words. So, let them speak. Instead of the current

super.getFoo()
        .foo()
        .getBar()
        .bar();

we now get the nicely aligned

super.getFoo()
     .foo()
     .getBar()
     .bar();

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 21, 2018
@tobiasdiez tobiasdiez changed the title ~~Django~~ style unchained. Django style unchained. Jan 21, 2018
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Good! Now we just need the same for the Eclipse style.

@lenhard
Copy link
Member

lenhard commented Jan 22, 2018

Code examples may be better than words, but only if they are really really obvious ;-) To be honest, I don't get why the new style is strictly better. I have nothing against it, but I also don't see any obvious benefit. To me, it just doesn't matter.

Could you convince me with words?

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Jan 22, 2018

Well, there are of course no real benefits. We will not perform 200% more efficient because of this change. It's just a matter of taste and for me fluent code (with builders or the stream api) does not look "nice". But of course this is a subjective opinion. For example, why is map indented (with respect to getValues) but collect is on the same level as map? It just feels unlogical.

return field.getValues().stream()
                .map(SpecialFieldValueViewModel::new)
                .collect(Collectors.toList());

The reformatted

return field.getValues().stream()
            .map(SpecialFieldValueViewModel::new)
            .collect(Collectors.toList());

is easier to read and to "parse" in my opinion. Get values, map them, collect. Finish.

@lenhard
Copy link
Member

lenhard commented Jan 22, 2018

Ok, I am buying into that :-) Could we do the same configuration for Eclipse here as well (as requested by @Siedlerchr ). Just to ensure that there is no constant reformatting of the code with different IDEs.

@Siedlerchr
Copy link
Member

https://stackoverflow.com/questions/6275785/wrapping-chained-method-calls-on-a-separate-line-in-eclipse-for-java
will try to check that the next days

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 22, 2018
@tobiasdiez tobiasdiez changed the title Django style unchained. [WIP] Django style unchained. Jan 22, 2018
@stefan-kolb stefan-kolb merged commit 834e6fa into master Feb 6, 2018
@stefan-kolb stefan-kolb deleted the styleUnchained branch February 6, 2018 11:23
stefan-kolb added a commit that referenced this pull request Feb 6, 2018
Siedlerchr added a commit that referenced this pull request Feb 6, 2018
* 'master' of github.com:JabRef/jabref:
  Allow spaces in DOIs
  Remove irrelevant log messages during XMP reading
  Adapt log4j configuration for cleaner junit tests #3511
  Eclipse Django style #3655
  Better code style for chained methods
  Update build.gradle
  Update build.gradle
  cleanup and refactoring in DuplicateCheck class
  code review fixes - consider pages of the same book separately in duplications detection process; add more tests
  differentiate inbooks with the same author and title, but different chapter
  Remove deprecated static BibtexParser.parse method
  Use stream in matcher (#3696)
  Add some BibtexNameFomatter comments
  Add exception for Jacoco
  Set jacoco toolVersion earlier
  set jacoco version globally
Siedlerchr added a commit that referenced this pull request Feb 7, 2018
* upstream/master: (47 commits)
  Fix Google Scholar fetcher
  Use english for all LCID mappings #1851
  remove import preferences test mock import and convert to junit5
  fix #3693 (#3702)
  Fix space leads to jump in entry editor (#3699)
  Fix #3669
  Extract creation of the contents of FileAnnotationTabController
  Fix changelog
  Fix NPE
  Remove unnecessary file
  use processbuilder for calling external apps
  Fix color highlight of odd linked files
  Fixes #2964
  Fix koppor issues #3
  Allow spaces in DOIs
  Remove irrelevant log messages during XMP reading
  Adapt log4j configuration for cleaner junit tests #3511
  Eclipse Django style #3655
  Better code style for chained methods
  Update build.gradle
  ...
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.

4 participants