Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upMigrate common name attributes upward #3524
Conversation
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 23, 2018
Awesome! You just created a pull request to the Citation Styles Language styles repository. One of our human volunteers will try to get in touch soon (usually within a week). In the meantime, I will run some automated checks. You should be notified of the results in a few minutes. If you haven't done so yet, please make sure your style validates and follows all our other Style Requirements. To update this pull request, visit the "Files changed" tab above, and click on the pencil icon (see below) in the top-right corner of your style to start editing. If you have any questions, please leave a comment and we'll get back to you. While we usually respond in English, feel free to write in whatever language you're most comfortable. |
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 23, 2018
|
This comment has been minimized.
This comment has been minimized.
Oops. It seems there is a problem with validation ... |
This comment has been minimized.
This comment has been minimized.
It's actually a problem with the Travis checks, not with the styles. Travis is demanding that if one of |
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
146ae2a
to
4af4fb1
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
This comment has been minimized.
This comment has been minimized.
Oh! I jumped to conclusions there, as I so often do. There is a specific problem with the When the script performs the migration, the However (ahem) the same macro is called from Removing (First draft of this post edited after more careful inspection.) |
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
4af4fb1
to
de7ebcc
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
de7ebcc
to
7034200
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
7034200
to
5ebef65
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
This comment has been minimized.
This comment has been minimized.
The concern here is readability. We've been adding the |
This comment has been minimized.
This comment has been minimized.
Yes---I'm not pushing at all. It's just something that I started tinkering with, and wanted to finish. When it's done and working, I can bundle the script in a project so that it's available for use if desired. |
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
5ebef65
to
d9217f8
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
d9217f8
to
96d3367
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
96d3367
to
4bd39d0
May 24, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 24, 2018
|
This comment has been minimized.
This comment has been minimized.
Well, that was an interesting journey. Tests finally pass! The script touches 1604 styles in all. The repeated test failures above were triggered by a series of sloppy-coding bugs that failed to preserve the pairing of Caution advised, but these should produce output identical to that of the originals. As to whether moving these attributes actually makes maintenance easier, that is one for active maintainers to call. I just wanted to give the script a proper test run once I had begun coding it. (If this one isn't adopted, feel free to close the pull request at will!) |
This comment has been minimized.
This comment has been minimized.
As a final thought, this could be extended to reverse the migration, removing all inheritable attributes and placing them always directly on |
This comment has been minimized.
This comment has been minimized.
As far as I understand, this script looks for the et-al settings in cs-name and if they're the same it actually puts it straight up in the first lines? |
This comment has been minimized.
This comment has been minimized.
I agree on the This goes further, though, and moves all inheritable elements there, i.e. also things like |
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 25, 2018
|
This comment has been minimized.
This comment has been minimized.
@adam3smith @POBrien333 @rmzelle Here you go. I've pushed a changeset limited to the |
This comment has been minimized.
This comment has been minimized.
As a side-note, more styles would be affected if |
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
a1703a6
to
076c373
May 26, 2018
This comment has been minimized.
This comment has been minimized.
I've run a little poke-it-and-see test, and I was wrong about the Travis logic. Either of |
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
45b0454
to
076c373
May 26, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
26eff65
to
076c373
May 26, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
fbennett
force-pushed the
fbennett:migrate-common-name-attributes-upward
branch
from
8a5609f
to
076c373
May 26, 2018
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
May 26, 2018
|
This comment has been minimized.
This comment has been minimized.
There we go. The third check-in touches just six additional styles, so there was a high degree of editorial consistency already (and no, that is not surprising!). Two styles were very slightly edited to avoid Travis errors: |
This comment has been minimized.
This comment has been minimized.
csl-bot
commented
Jul 1, 2018
|
adam3smith
merged commit 69304f7
into
citation-style-language:master
Jul 1, 2018
1 check passed
This comment has been minimized.
This comment has been minimized.
Cool! CC @POBrien333 -- no more unnecessary et als on names! |
fbennett commentedMay 23, 2018
•
edited
Not sure how this one will be received ...
This is a more ambitious set of changes than #3517. It removes inheritable attributes from
cs:names
andcs:name
nodes if they are the same in all calls originating from theircs:citation
orcs:bibliography
parent, and if they do not conflict across the two parents, and sets them on the parent instead.The advantage of the reprocessed styles is a reduction in code replication. Also, since all inheritable attributes that remain on
cs:names
andcs:name
nodes are consequential, it makes it easier to spot discrepancies, such as disparate application ofinitialize-with
.Here is the script used to apply the changes, with notes on the logic of the scan. (If this full pull request seems overly ambitious, the script can be applied to individual styles as well.)
(Code below is the working code, edited since the initial post here went up.)