You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@lucenenet.apache.org by NightOwl888 <gi...@git.apache.org> on 2017/01/25 06:29:30 UTC

[GitHub] lucenenet pull request #203: API Work - Stabilization

GitHub user NightOwl888 opened a pull request:

    https://github.com/apache/lucenenet/pull/203

    API Work - Stabilization

    This is a branch of #191 and supersedes that pull request. To build this branch, see the instructions on #191.
    
    The plan is to work on the API here to keep from releasing several successive releases with breaking API changes. Once the API is relatively stable, we can merge to master and create a release that can be consumed without having to worry about so many breaking API changes between releases.
    
    > DO NOT MERGE this pull request until the API is stable and failing tests are addressed.
    
    ## API Polishing
    
    This fixes several API issues that were making Lucene.Net deviate from looking like a normal .NET application - mainly by using properties and methods the way that are specified in the [MSDN guidelines](https://msdn.microsoft.com/en-us/library/ms229054(v=vs.100).aspx) and following .NET naming conventions for properties, methods, fields, parameters, and types. Also several types are being renamed and/or moved to address naming collisions, incorrect porting corrected, bugs fixed, and other issues such as CLS compliance are being addressed.
    
    These specific changes are being made that make Lucene.Net differ from Lucene because they are framework conventions that are different between Java and .NET:
    
    1. Interfaces start with "I"
    2. `Size()` changed to `Count` or `Length` property
    3. `Comparator` changed to `Comparer` on all classes, interfaces , properties, methods, documentation and comments
    4. Type names in methods and properties being .NETified. For example instead of `.SetLongValue(long)`, we will have `.SetInt64Value()`. NOTE: This isn't done yet. I could use some advice on if or how this one should be done, since `SetVInt64()` admittedly looks odd. Clearly, we can't change the class and interface names to match because there are types in the same context where `Single` means "singular" rather than the `float` data type, and it would be confusing, but in .NET the names don't match anyway (`Convert.ToInt64()` returns type `long`) so I think that would be okay.
    
    ## Status
    
    #### Key:
    
    1. Type and Member Accessibility
    2. Properties vs Methods
    3. Properties vs Fields
    4. Method, Property, Field, and Parameter Naming Conventions
    5. Class and Interface Naming Conventions
    6. No Nullable Enums
    7. Parameter and Return Types (IDictionary and IList rather than Dictionary and List)
    8. CLS Compliant
    9. Compiler Warnings Remaining (corresponding test project included)
    10. TODOs Remaining (search for `LUCENENET TODO:` or `LUCENE TO-DO` to find them)
    -------
    
    | Project | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 |
    |  :---      | :---: | :---: | :---: | :---: | :---: | :---: | :---: | :---: | :---: | :---: | 
    | Core                       | X |  | X |  | X |  | ? |  | 109 | 267 |
    | Analysis.Common  |  |  |  |  |  | X | ? | X | 22 | 23 |
    | Analysis.Stempel    | X | ? | ? | ? | ? | ? | ? | X | 0 | 0 |
    | Classification          |  | ? | ? | ? | ? | ? | ? | X | 0 | 0 |
    | Codecs                   | ? | ? | ? | ? | ? | ? | ? | X | 0 | 5 |
    | Expressions            |  | ? | ? | ? | ? | ? | ? | X | 0 | 0 |
    | Facet                      |  X | ? | ? | ? | ? | ? | ? | X | 6 | 14 |
    | Grouping               |  X | ? | ? | ? | ? | ? | ? | X | 0 | 8 |
    | Highlighter            |  X | ? | ? | ? | ? | ? | ? | X | 0 | 6 |
    | Join                        |  | ? | ? | ? | ? | ? | ? | X | 0 | 1 |
    | Memory                 |  X | ? | ? | ? | ? | ? | ? | X | 0 | 0 |
    | Misc                       |  X | ? | ? | ? | ? | ? | ? | X | 8 | 6 |
    | Queries                  |   | ? | ? | ? | ? | ? | ? | X | 0 | 3 |
    | QueryParser           |  X | ? | ? | ? | ? | ? | ? | X | 2 | 20 |
    | Sandbox                 |  X | ? | ? | ? | ? | ? | ? | X | 0 | 0 |
    | Spatial                    |  X | ? | ? | ? | ? | ? | ? | X | 0 | 3 |
    | Suggest                  | X | ? | ? | ? | ? | ? | ? | X | 0 | 4 |
    | Test Framework      | N/A | N/A | N/A |  N/A | N/A | N/A | N/A | N/A | 4 | 5 |
    
    ### Tests
    
    Do note that currently this branch has more failing tests than master. In fact, the test suite is currently crashing for several tests, making the whole thing bomb before it can finish. Right now, the priority is finishing the API changes before addressing these problems. 
    
    However, if you would like to help out by debugging the failing tests, please fork [this branch](https://github.com/apache/lucenenet/tree/api-work) and submit the pull request back here. Due to the fast changing state of the API, please fix one bug at a time and submit the pull request to [this branch](https://github.com/apache/lucenenet/tree/api-work) (not master) ASAP.
    
    ### Documentation Comments
    
    Also up-for-grabs is to complete the documentation comments in the following projects.
    
    1. Core
    2. Analysis.Common
    3. Classification
    4. Expressions
    5. Grouping
    6. Join
    7. Queries
    
    While it is assumed that the documentation comments for the other projects are finished, they could probably all use a review.
    
    The automated Java converter brought much of the comment structure over, however some comments have been left behind in Lucene, and they need to be formatted and corrected to match [MSDN's guidelines](https://msdn.microsoft.com/en-us/library/5ast78ax.aspx). Some common issues that need to be addressed:
    
    1. Change `seealso` to `see` (`seealso` are for the links that go at the bottom of the documentation page, not the direct links within the content).
    2. `{@code paramName}` or `<code>paramName</code> need to be changed to `<paramref name="paramName">`. Note that not all of the "code" are parameter names, and the normal case need to be in `<c></c>` tags for single line and `<code></code>` tags for multiline code examples. Do note the code also needs to be converted to match our API.
    3. `{@link TypeName}` need to be changed to the appropriate `<see cref="TypeName"/>` or anchor tag (if external).
    4. Many times in the documentation, the types are referred to (in Pascal case) with no link. In these cases a link to the type should be added (change `TypeName` to `<see cref="TypeName"/>`).
    
    > Be sure to check the comments against [Lucene 4.8.0](https://github.com/apache/lucene-solr/tree/releases/lucene-solr/4.8.0/lucene) to ensure they are correct and complete!
    
    Pull requests should be directed to [this branch](https://github.com/apache/lucenenet/tree/api-work) (not master).


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/lucenenet api-work

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucenenet/pull/203.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #203
    
----
commit 956ecf044c0e5c22cf63f46a351e379adb0e7c7b
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:02:44Z

    Lucene.Net.Core.Util.AttributeSource refactor: renamed private/internal fields camelCase

commit 36a07f671994692727ead370658f535f3024e617
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:03:50Z

    Lucene.Net.Core.Util.CollectionUtil refactor: renamed private/internal fields camelCase

commit aca3935fd6a110d5f2f5f96026d17877defbd23a
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:04:25Z

    Lucene.Net.Core.Util.Counter refactor: renamed private/internal fields camelCase

commit 98e709aaddbe0726789692731ba7af5fef35c8b6
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:04:58Z

    Lucene.Net.Core.Util.DocIdBitSet refactor: renamed private/internal fields camelCase

commit f7beea039ea9512b3363be21c22859c783db6936
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:06:04Z

    Lucene.Net.Core.Util.FixedBitSet refactor: renamed private/internal fields camelCase

commit a07f85eb9e836fc837a9992289d828a05b1c27e4
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:07:05Z

    Lucene.Net.Core.Util.IndexableBinaryStringTools refactor: renamed private/internal fields camelCase

commit 13285db6a5e9d4dad6bbb44eedf18fa9c7e7581f
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:07:47Z

    Lucene.Net.Core.Util.OfflineSorter refactor: renamed private/internal fields camelCase

commit 770291e992490a6fb3a33ba872e8575e58b49f9c
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:23:44Z

    LuceneTestCase: Modified field scan to test field name for < to identify auto-implemented properties.

commit a8a716a738da3404edf7273a954a67098a69bb1e
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:25:47Z

    Lucene.Net.Util.Fst.FST.ArcAndState refactor: changed internal fields to properties

commit 62428a057e4b231dc7e5708d73c7ebb5a0347ce8
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:27:07Z

    Lucene.Net.Util.Fst.Util refactor: renamed private/internal fields camelCase

commit 847e48404b4a9fc1833fb9bdf6a40648ef9280e4
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:29:24Z

    Lucene.Net.Util.Automaton.BasicOperations refactor: renamed private/internal fields camelCase

commit 57422157067bca9f0b9bb851815d9381966869e1
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:30:46Z

    Lucene.Net.Store.CompoundFileDirectory refactor: renamed private/internal fields camelCase

commit 56fa68b51a5e1cd0f92a094f9d581af247aa5e24
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:31:52Z

    Lucene.Net.Store.CompoundFileWriter refactor: renamed private/internal fields camelCase

commit 42404dcb0b7bd363c0220c85358995e0d8855c6d
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:33:02Z

    Lucene.Net.Store.FSDirectory refactor: renamed private/internal fields camelCase

commit 79dca6e5df09b18e290cff9816adc7a204023335
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:33:37Z

    Lucene.Net.Store.Lock refactor: renamed private/internal fields camelCase

commit 002c01c0c779677c21647bd2a0fbbf7a16f8c3db
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:35:07Z

    Lucene.Net.Search.BooleanQuery refactor: renamed private/internal fields camelCase

commit 226473f3b9ba91908960d228d5a05d2536b8fbcf
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:36:16Z

    Lucene.Net.Search.BooleanScorer refactor: renamed private/internal fields camelCase

commit 6097448be962a835fa274be0e47c503302dfa2cb
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:37:53Z

    Lucene.Net.Search.BooleanScorer2 refactor: renamed private/internal fields camelCase

commit f208331fe7978fdb060b06f32310aee9ea42425e
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:38:28Z

    Lucene.Net.Search.ConjunctionScorer refactor: renamed private/internal fields camelCase

commit b340368a7e792f6a638c8f138fddda1fa520e40b
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:39:57Z

    Lucene.Net.Search.ConstantScoreAutoRewrite refactor: renamed private/internal fields camelCase

commit cbae07a115d7e616a5b769a977499722f68faaa8
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:40:44Z

    Lucene.Net.Search.SortRescorer refactor: renamed private/internal fields camelCase

commit e3a0c3d3786d4d3e5852312971a91c9ba182781c
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:42:17Z

    Lucene.Net.Search.Spans.SpanOrQuery refactor: renamed private/internal fields camelCase

commit ab3570c5ac010df08b58571dc66fbc49cc6db16e
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:45:36Z

    Lucene.Net.Index.ConcurrentMergeScheduler refactor: renamed private/internal fields camelCase

commit 553959d46c1a1ba1c5c3987e4975baa1549329e9
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:49:14Z

    Lucene.Net.Index.DocTermOrds refactor: renamed private/internal fields camelCase

commit 0bc588f7d78c5d83d6f86485bf5be16625b9f930
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:51:31Z

    Lucene.Net.Index.DocValues refactor: renamed private/internal fields camelCase

commit 135de260199ccbcff38f525fd7b7ac0423d63bc8
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:52:21Z

    Lucene.Net.Index.MultiDocValues refactor: renamed private/internal fields camelCase

commit d15d9c78aeed4ff64b51dddb4a88bf03ccc22a54
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:53:06Z

    Lucene.Net.Index.PrefixCodedTerms refactor: renamed private/internal fields camelCase

commit 1bfcb5c59ca2143947ca82225ae83122618f3b74
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:55:12Z

    Lucene.Net.Documents.Field refactor: renamed private/internal fields camelCase

commit 0adb11baf81265825e7ab14d9ba784ef902f363b
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:56:07Z

    Lucene.Net.Core.Codecs.DocValuesConsumer refactor: renamed private/internal fields camelCase

commit 9af294db60a33531b444d1faf714267db0855e60
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date:   2017-01-04T14:58:25Z

    Lucene.Net.Core.Codecs.PerField.PerFieldDocValuesFormat refactor: renamed private/internal fields camelCase

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

https://github.com/apache/lucenenet/pull/203

I also apologize for the delayed response.

> Is there anything I can help with?

Yes, there is. I have been working on cleaning the API up and making it more .NET-like.

### CLS Compliance

Part of this process is making the whole project CLS compliant, so it can be consumed by VB and other .NET languages.

1. Currently, there are only 8 remaining CLS-related warnings that need to be addressed. But before we fix them, it would be nice if we could ensure that the fixes don't cause any negative effects - in other words, we need as many tests fixed as possible (namely in Lucene.Net.Core). If you could help with the debugging efforts, that would be great.
2. In addition, there was a bit of a snag with icu-dotnet because *it* is not marked CLS Compliant. Therefore, by default none of its types are CLS Compliant and cannot be exposed publicly in Lucene.Net (including Enum types). I would appreciate it if you mark the assembly with the CLSCompliant attribute.

I reverted the ThaiAnalyzer back to its previous state and adapted the ICUBreakIterator to work with it - seems to work pretty well, for now (and it is now CLS Compliant). Let me know if/when you have a rule-based break iterator so I can ensure compatibility.

### Collation

Also, I ended up marking the Collation-related types CLSCompliant(false). I am now pretty much convinced that there is no way we can get them working with icu-dotnet, based on [this documentation](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/4.8.0/lucene/analysis/common/src/java/org/apache/lucene/collation/CollationAttributeFactory.java#L65-L68). It also seems to indicate that the collation functionality in the Analysis.ICU package is better (and it is based on ICU 4J, so it should work). Anyway, my thought is to add `FEATURE_COLLATION` as an option and leave it out of the compile, since it is pretty much useless unless we port over a large part of the JDK to make it work. So, ignore those failing tests for the time being.

### Test Framework

I also reverted the changes to OLD_FORMAT_IMPERSONATION_IS_ACTIVE and made it static again. There really doesn't seem to be a reasonable way to ensure this flag is read from everywhere if it is non-static. This fixed more than a dozen failing tests. I am working on finishing up the Codec functionality now - specifically the randomizing of codecs and the IgnoreCodecs functionality.

But, I could use some help. The test framework has been a major source of test failures. Many parts of it were not translated correctly or are still unfinished. IMO, the test framework should be reviewed line-by-line to ensure that the equivalent functionality is implemented.

### Documentation Comments

Also, as I [mentioned above](https://github.com/apache/lucenenet/pull/203#issue-203023160), some of the documentation comments need to be finished. Mostly, they are malformed so they don't show up in Visual Studio, but there is also some documentation that is either incorrect or has not been brought over from Lucene.

---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by davhdavh <gi...@git.apache.org>.

Github user davhdavh commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    After trying to get strong names on, I can say that the entire build system is broken on latest version of dotnet sdk. 1. Restore needs a specific sln, since there are two, ie: `& dotnet.exe restore $base_directory\Lucene.Net.sln` 2. build command no longer accepts a project.json, but instead uses the csproj files. That can be fixed by finding the csproj files before build-assemblies call:
    ```
    		pushd $base_directory
    		$projects = Get-ChildItem -Path "*.csproj" -Recurse
    		popd
    
    		Build-Assemblies $projects
    ```
    and yet with those fixes, I still cant get it working.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    @davhdavh - I have now added the XML documentation to the packages.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by davhdavh <gi...@git.apache.org>.

Github user davhdavh commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    Excellent. I notice that the .xml files are still not in the nuget packages, so no api docs?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by davhdavh <gi...@git.apache.org>.

Github user davhdavh commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    Thanks for all the quick fixes :) I found two more issues. 1. The assemblies are not strong-named in either debug or release build. 2. The assembly version is 4.0.0.0. I believe the right thing to do would be to set `[assembly: AssemblyVersion("4.8.*")]` and `[assembly: AssemblyInformationalVersion("4.8-apiwork-ci")]` 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    Nope, there is still a lot of broken XML in the documentation comments (see above) that are preventing the process from being complete. It would take a single person the better part of a week to correct this - it would sure be nice if others participated. But now that you mention it, I could probably turn the option to produce the XML on for all but the 2 incomplete packages at least.
    
    BTW - I am legally obligated to tell you (thanks to Apache) that those packages are not for production release.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by davhdavh <gi...@git.apache.org>.

Github user davhdavh commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    A small bug in the port in `Term.ToString(BytesRef termText)`. The call to `termText.Utf8ToString(); ` will never throw, it should use the system utf8encoder instead: `return new System.Text.UTF8Encoding(false, true).GetString(termText.Bytes, termText.Offset, termText.Length);`. but IHMO, it is still fundamentally broken, ran into this on an int that happens to have a valid utf8 encoding when serialized to bytes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by conniey <gi...@git.apache.org>.

Github user conniey commented on the issue:

https://github.com/apache/lucenenet/pull/203

@NightOwl888 I apologise for the delayed response to your NUnit tests. I've been working on another project. Is there anything I can help with?

My current tasks for Lucene.NET that I would want to merge into this PR are:

**Icu-dotnet**

* Bring BreakIterator changes into local icu-dotnet .NET Core branch.
* Use icu-dotnet's new way of loading NativeLibrary dependencies
* There have been a lot of churn in the icu-dotnet repository since I created my issue about migrating to .NET Core. They changed the way that they ingested Native library dependencies and split it into a different repository that I have to merge to and take into account.
* Fix assembly loading changes in .NET Core
* Recently, icu-dotnet introduced a new way of loading assemblies that uses native Windows/Linux API calls rather than using a [DllImport] from C#. Since we implemented a new way to load native libraries in .NET Core (via /runtimes/{RID}/ rather than adding to the Environment.Path), it breaks loading in .NET Core. I need to research how to get the native library runtime paths in .NET Core. Then figure out how to load it first when they are using the Windows/Linux APIs
* Modify [BreakIteratorRules.java](http://www.docjar.com/html/api/sun/text/resources/BreakIteratorRules.java.html) to consumable rules for icu-dotnet

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    @davhdavh - thanks for the report. I was wondering about that piece of code, but since none of the tests were complaining I left it alone. Thanks for complaining in their place. This is just another place where someone deviated from the original code that happened to be the wrong choice (as usually is the case).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    @davhdavh 
    
    The merge will be happening soon. @synhershko, do you need to review this before merging, or can the review be done after? I am ready to merge when you are.
    
    As for x64 support, I believe we do have it now that we have switched from ICU4NET to [icu-dotnet](https://github.com/sillsdev/icu-dotnet), but @conniey might be able to answer that for sure. Also, the ICU-specific functionality has been moved to its own package so the majority of users don't need to deal with that dependency anymore.
    
    This branch is now being continuously deployed to https://www.myget.org/Gallery/lucene-net-ci, where you can check out the packages for yourself. Please let me know if there are any issues utilizing them.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] lucenenet issue #203: API Work - Stabilization

Posted by NightOwl888 <gi...@git.apache.org>.

Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/203
  
    @davhdavh 
    
    > 1. The assemblies are not strong-named in either debug or release build.
    
    Per Itamar (the project manager), [Lucene.Net will not be strong-named going forward](http://code972.com/blog/2014/04/68-ditching-strong-naming-for-lucene-net). I don't agree with all of his points, but for now I am just going to see how many people complain. I did strong-name the [`spatial4n` dependency](https://github.com/NightOwl888/Spatial4n/) just in case we need to do it. He might be right that it is time to ditch it as some other open-source projects have. Personally, I don't need it - do you? If so, I suggest you complain loudly about it on the [dev mailing list](https://cwiki.apache.org/confluence/display/LUCENENET/Mailing+Lists) and open an issue on [JIRA](https://issues.apache.org/jira/browse/LUCENENET-574?jql=project%20%3D%20LUCENENET%20AND%20status%20%3D%20Open) about it so we can see how many votes it gets.
    
    > 2. The assembly version is 4.0.0.0. I believe the right thing to do would be to set [assembly: AssemblyVersion("4.8.*")] and [assembly: AssemblyInformationalVersion("4.8-apiwork-ci")]
    
    In case we do strong-name because of popular demand, setting the assembly version to 4.0.0.0 is the right way to go. This is what the MVC team did - all versions of MVC 4 were 4.0.0.0 (until a they found a security vulnerability so severe that they had to force everyone to upgrade to it, then it incremented to 4.0.0.1). If you read the [SemVer](http://semver.org/) document, the behavior of strong-naming acts exactly like changing the major version, since whenever it is changed it breaks binary compatibility. Therefore, it should never change unless the major version is changed.
    
    > After trying to get strong names on, I can say that the entire build system is broken on latest version of dotnet sdk. 1. Restore needs a specific sln, since there are two, ie: & dotnet.exe restore $base_directory\Lucene.Net.sln 2. build command no longer accepts a project.json, but instead uses the csproj files.
    
    If you look at the [`global.json` file](https://github.com/apache/lucenenet/blob/master/global.json), the build requires the `1.0.0-preview2-1-003177` SDK, which is the [current one](https://github.com/dotnet/core/blob/master/release-notes/download-archive.md) that supports `project.json` (but now that you mention it, the README needs to state the prerequisites). The `global.json` file ensures the right SDK is used even if a newer one is installed.
    
    I made an attempt to unify to one solution and upgrade to `.csproj`, but ran into many issues:
    
    1. The NUnit test adapter doesn't yet support `.csproj` on .NET Core, so no debugging in VS2017 on .NET Core unless you fire off the tests manually or with NUnitLite.
    2. Some of the tests would not complete after the switch.
    3. With `.csproj`, versioning is still broken with respect to what I mentioned above about the assembly version (which I plan to file an issue about) - when you specify an AssemblyVersion, it always uses the same version for the AssemblyFileVersion, meaning they cannot differ. Also, it is broken in respect to using a non SemVer scheme (which I don't see an alternative for a port, since we will almost certainly have multiple production releases that correspond to Lucene 4.8.0). You cannot pass a version number to the `dotnet pack` command, only a version suffix (which always assumes there will be a `-` before it and always assumes there will be a version prefix). In addition, when generating the NuGet packages for a pre-release, it doesn't update the project dependency version numbers to pre-release (which gives you compile warnings).
    
    These issues can probably be overcome and I much prefer the new format, but I didn't want to delay the release any longer. I think it would be best to wait until Microsoft announces they are feature-complete  and NUnit Test Adapter has .NET Core support rather than trading one broken build system for another.
    
    I wouldn't object if you want to contribute a build option to turn on strong-naming (provided it doesn't break the build in TeamCity). But keep in mind, many of the assemblies are using `InternalsVisibleTo` so we will need a conditional compilation symbol (`FEATURE_STRONGNAME`) to toggle them between the standard and strong-named form. The strong-name option would not necessarily need to extend to the `Lucene.Net.sln` solution or its projects, since they are not used for the build.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---