Rename getLineNo() to getLineNumber()

[oliverklee]

• add getLineNumber as an alias in V8.x
• deprecate getLineNo in V8.x
• rename it in main

[JakeQZ]

Partly done: #1225, #1233.

[JakeQZ]

The original deprecation and other related changes may need reworking.

Rule::addRule() sets the column number to one less than the sibling. Which means the default column number must be 0 not null, and that column numbers may be negative. Not sure about line number, but it's used in sorting, so again having a null default may be problematic. I think we need to revert back to using 0 as defaults for both (and not allow null), and backport this, with the end result that getLineNumber and getColumnNumber are just aliases to the pre-existing abbreviatedly-named methods.

Fortunately, we have not yet made a release with #1232 included.

[oliverklee]

Rule::addRule() sets the column number to one less than the sibling.

If the sibling has null as the column number (which means "no column number known), the right thing (as I understand it) would be to set the column number of the other node to null as well. (We'd then need to take null into account when sorting as well.)

[JakeQZ]

If the sibling has null as the column number (which means "no column number known), the right thing (as I understand it) would be to set the column number of the other node to null as well.

Yes, I think that's the best we can do.

(We'd then need to take null into account when sorting as well.)

Yeah, I'll need to take a look at how the sorting works. It's possible that the change to nullable was slightly breaking (though perhaps null is now just getting automatically cast to 0).

The question arises: how should Rules with null position be sorted against Rules with a defined position? (I think the nulls should go at the end.)

That said, I think (usually) Rules always end up with a defined position, either from parsing, or when added (if $sibling parameter not provided, the Rule is assigned a line number one more than the current highest - though need to check what happens when there are no existing Rules). Except for setRules() which could set Rules without position information.

Rule::addRule() sets the column number to one less than the sibling.

This is problematic anyway if adding more than one Rule, but beyond the scope of this issue.

Hmm. This initially seemed like a straightforward change !))

[oliverklee]

I have another idea: We could enforce that all Rules within a RuleSet have a defined position. Maybe like this:

1. in setRules(), throw an exception if one of the set rules does not have a position
2. deprecate setRules() to be removed (people should use addRule() instead
3. remove setRules()

WDYT?

[JakeQZ]

I have another idea: We could enforce that all Rules within a RuleSet have a defined position.

I think we should do this, with both line and column number required.

Looking at the code now, setRules() calls addRule() for each Rule in the array passed. So provided that addRule() always ensures a fully valid position, we don't need to change or deprecate setRules().

addRule(), however, does not currently ensure a valid position for the first Rule added, though this is easy to fix.

I also note that addRule() with a sibling does not seem to set the Position of the Rule just before the sibling if it has a different property name. If that's the case, it's a bug - being able to insert, say, a font Rule before a font-size Rule is important. It is also easy to fix.

To avoid negative and/or overlapping column numbers, addRule() with a sibling could be changed to increment the column number of the sibling and all other Rules with the same line number and a higher column number, before adding the Rule with the original line and column number of the sibling.

I think the next step is to add the remaining unit tests for RuleSet, including checking that parseRuleSet(), addRule() and setRules() always result in all Rules having a valid position. This is needed for #1194 anyway to be able to test the delegation methods (by ultimately moving the tests to a trait). It should be possible to find some failing cases for the issues mentioned above.

[oliverklee]

Sounds like a good plan to me!